Skip to content

Add a geometry from yaml constructor for Wing #186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

1-Bart-1
Copy link
Member

@1-Bart-1 1-Bart-1 commented Aug 4, 2025

No description provided.

Copy link

codecov bot commented Aug 4, 2025

Codecov Report

❌ Patch coverage is 88.55422% with 19 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/yaml_geometry.jl 91.30% 8 Missing ⚠️
src/plotting.jl 70.83% 7 Missing ⚠️
src/VortexStepMethod.jl 0.00% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

@1-Bart-1
Copy link
Member Author

1-Bart-1 commented Aug 5, 2025

@jellepoland when you feel like this pull request is ready, you can convert it from a draft pull request to a normal pull request by marking it ready for review, and request a review from me and/or uwe.

Copy link
Member Author

@1-Bart-1 1-Bart-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool stuff, thanks for the contribution! 🚀 Left some comments, please address these.

@jellepoland
Copy link
Collaborator

Great comments!

My main takeaway is the struct--constructor interaction in Julia, that I am not used to in Python.The defining of a single struct, and multiple functions with the same name as that struct (e.g. Wing) all being able to take in different input types, and Julia then automatically picking the appropriate functions.
Pretty neat I have to say :)

@1-Bart-1
Copy link
Member Author

1-Bart-1 commented Aug 7, 2025

On which branch did you address my comments? I don't see it here...

@jellepoland
Copy link
Collaborator

I am still working on the changes!

I am changing and extending the test directory; to be more extensive, readable and logically structured.

@jellepoland
Copy link
Collaborator

Reorganized the testing suite and added some tests.
More could be added still, see #191.

@1-Bart-1
Copy link
Member Author

1-Bart-1 commented Aug 7, 2025

Getting a lot of these warnings in the tests, please fix these before we merge this branch @jellepoland

WARNING: Method definition test_data_path(Any, Any...) in module Main at /Users/runner/work/VortexStepMethod.jl/VortexStepMethod.jl/test/test_data_utils.jl:23 overwritten on the same line (check for duplicate calls to `include`).
WARNING: Method definition create_temp_wing_settings(Any, Any) in module Main at /Users/runner/work/VortexStepMethod.jl/VortexStepMethod.jl/test/test_data_utils.jl:46 overwritten on the same line (check for duplicate calls to `include`).
WARNING: Method definition kwcall(NamedTuple{names, T} where T<:Tuple where names, typeof(Main.create_temp_wing_settings), Any, Any) in module Main at /Users/runner/work/VortexStepMethod.jl/VortexStepMethod.jl/test/test_data_utils.jl:46 overwritten on the same line (check for duplicate calls to `include`).
WARNING: Method definition get_standard_wing_file(Any) in module Main at /Users/runner/work/VortexStepMethod.jl/VortexStepMethod.jl/test/test_data_utils.jl:103 overwritten on the same line (check for duplicate calls to `include`).
WARNING: Method definition get_complete_settings_file(Any) in module Main at /Users/runner/work/VortexStepMethod.jl/VortexStepMethod.jl/test/test_data_utils.jl:125 overwritten on the same line (check for duplicate calls to `include`).

Copy link
Member

@ufechner7 ufechner7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, fix the failing tests on windows and the "method overwritten" warnings first.

Furthermore, add a description that explains what you changed.

@ufechner7 ufechner7 requested a review from Copilot August 7, 2025 21:56
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds YAML-based wing geometry construction capabilities to VortexStepMethod.jl, enabling users to define wings through configuration files with embedded polar data references.

Key changes:

  • Implement yaml_geometry.jl module with Wing constructors that read YAML configuration files
  • Add support for loading aerodynamic polar data from CSV files referenced in YAML
  • Reorganize test suite into module-specific directories with comprehensive test coverage

Reviewed Changes

Copilot reviewed 73 out of 111 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/yaml_geometry.jl Core YAML geometry parsing with Wing constructors and polar data loading
test/yaml_geometry/ Comprehensive test suite with sample YAML/CSV files for wing geometry
test/test_data_utils.jl Shared utilities for test data management across modules
src/VortexStepMethod.jl Export new YAML geometry functionality
examples/ Example scripts demonstrating YAML-based wing construction
data/ Sample configuration files for pyramid and V3 kite models

@jellepoland
Copy link
Collaborator

jellepoland commented Aug 8, 2025

Summary of Key Changes from This Branch

1. Core New Functionality: YAML Geometry Support

  • New file: yaml_geometry.jl (290+ lines) - Complete YAML-based wing geometry loading
  • New structs: WingSectionData, WingAirfoilData, WingAirfoilInfo with @with_kw macros
  • New function: load_polar_data() - Robust CSV polar data loading with error handling
  • New constructor: Wing(geometry_file::String) - Creates wings from YAML files
  • New constructor: Wing(settings::VSMSettings) - Creates wings from settings

2. Enhanced Settings System

  • Renamed vs() to VSMSettings() constructor
  • Added convenience Solver(body_aero, settings) constructor
  • Improved settings structure and validation

3. Comprehensive Test Infrastructure

  • Split tests: Reorganized from monolithic files to modular structure (test/module_name/test_*.jl)
  • New test module: yaml_geometry with test_load_polar_data.jl and test_wing_constructor.jl
  • Test utilities: test_data_utils.jl with shared helper functions
  • Test data: Extensive YAML and CSV test files in data

4. Data and Examples

  • New data sets: Complete TUDELFT_V3_KITE with CFD polars and literature results
  • Enhanced examples: Updated examples to use YAML geometry (e.g., pyramid_model.jl, V3_kite.jl)
  • Real-world configs: Production-ready YAML geometry files for various kite configurations

5. Core Module Improvements

  • Path handling: Robust file path resolution for relative/absolute paths
  • Error handling: Comprehensive validation and graceful fallback to INVISCID mode
  • Memory management: Improved file I/O and cleanup in tests
  • Documentation: Added comprehensive docstrings and examples

@jellepoland
Copy link
Collaborator

Windows test passing

@jellepoland jellepoland requested a review from ufechner7 August 8, 2025 10:04
Copy link
Member Author

@1-Bart-1 1-Bart-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be merged now, @jellepoland can you update NEWS.md?

@jellepoland
Copy link
Collaborator

I updated the News.md

…xstepmethodjl-and-run-on-a-simple-wing' of github.com:OpenSourceAWE/VortexStepMethod.jl into 183-create-a-load_geometry_from_yaml-function-for-vortexstepmethodjl-and-run-on-a-simple-wing
Copy link
Member

@ufechner7 ufechner7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I approve it, please still find the time to answer my questions/ address my comments.

@@ -0,0 +1,9 @@
alpha,beta,CL,CD,CS,CMy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is CMy ?

project_dir = dirname(dirname(pathof(VortexStepMethod))) # Go up one level from src to project root

# Load VSM vsm_settings from YAML configuration file
vsm_settings = VSMSettings("pyramid_model/vsm_settings.yaml")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a pyramid model?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a model that has the shape of a pyramid :) You can run the example and it will plot it.

end

"""
load_polar_data(csv_file_path::String) -> Tuple{Union{Nothing, Tuple{Vector{Float64}, Vector{Float64}, Vector{Float64}, Vector{Float64}}}, Symbol}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you return such a complicated structure, isn't it better to define a struct type for it and just document that you return a PolarData struct or how ever you call it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants