Skip to content
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

Features/alignment optimization #96

Merged
merged 165 commits into from
Dec 16, 2024
Merged

Conversation

MarleneBusch
Copy link
Collaborator

Features/alignment optimization

This branch mainly introduces the AlignmentOptimizer that can be used to optimize all 28 heliostat kinematic parameters. There are two optimization methods, one uses the motor positions and the other uses raytracing. Optimizing via the motor positions is significantly faster and also more accurate.

This branch also changed other modules.

  • The tutorials were not updated in my last feature branch concerning the device, now the default device is also set there.
  • The h5 scenarios were updated to also include the power plant position coordinates.
  • The actuator used to have a forward method that had no use, i removed it.
  • The kinematic module variable names were not consistent, we used to have actuator_steps, motor_steps, actuator_pos, ... all were referring to the same thing that is named motor_positions in PAINT, thats why I chose that name for ARTIST as well.
  • A new pytest is introduced checking the util method that converts wgs84 to enu.
  • In the heliostat module, the get_surface_points_and_surface_normals method was moved from the alignement method to the init. This way it is only called once and not with each new alignment.
  • Inplace operations in the kinematic and raytracing were removed to make the modules differentiable.
  • A PAINT_to_surface converter was added to use PAINT data and create scenarios from it.
  • Overall I also found pieces of code that used the math library, I changed that to torch.

Fixes #91

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@MarleneBusch
Copy link
Collaborator Author

I added a lot of tests to cover all raise blocks.
I merged the stral and paint to surface converters into a single surface converter.
I removed all strings to read paint data and used mapping instead.

Copy link
Member

@kalebphipps kalebphipps left a comment

Choose a reason for hiding this comment

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

The number of changed files seems to keep exponentially growing! This is definitely a huge contribution!

Thanks for all the changes - especially all the tests to cover errors and the additional unit tests make a huge difference. I also like the way the SurfaceConverter is refactored.

I have included a few comments - most are minor points regarding DocStrings or things that we should maybe note as Issues to deal with later. I think the two important things are:

  • A small suggestion to further refactor the SurfaceConverter.
  • Some notes on files being included from test coverage. I know they were previously excluded, but looking through the code I think there might still be some things we can test there.

I also noticed that the updated keys for the PAINT data have not yet been included - we should probably update these (i.e. sun_azimuth instead of Sun_azimuth) and fix the test data before merging right?

Locally (I will push now) I have formatted the test .json files to be "pretty" and also included a numbering for the alignment tutorial in the file name.

Thanks again for all the work - and sorry for the further requested changes...there are so many changes that I keep noticing new things when I review it again...

artist/util/config_dictionary.py Outdated Show resolved Hide resolved
artist/field/heliostat.py Show resolved Hide resolved
artist/raytracing/rays.py Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
artist/util/surface_converter.py Outdated Show resolved Hide resolved
artist/util/surface_converter.py Show resolved Hide resolved
artist/util/surface_converter.py Show resolved Hide resolved
artist/util/utils.py Show resolved Hide resolved
artist/util/utils.py Show resolved Hide resolved
Copy link
Member

@kalebphipps kalebphipps left a comment

Choose a reason for hiding this comment

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

I have made a few changes to the docstrings and test coverage locally which I will now push.

It looks good to me though - I am happy to allow the merge!

@kalebphipps kalebphipps requested a review from mcw92 December 16, 2024 09:32
@kalebphipps kalebphipps merged commit f3a981c into main Dec 16, 2024
15 checks passed
@kalebphipps kalebphipps deleted the features/alignment_optimization branch December 16, 2024 10:41
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.

Alignment Optimization
3 participants