-
Notifications
You must be signed in to change notification settings - Fork 7
Features/multi heliostats #113
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
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
==========================================
+ Coverage 98.28% 98.34% +0.05%
==========================================
Files 30 30
Lines 1635 1692 +57
==========================================
+ Hits 1607 1664 +57
Misses 28 28 ☔ View full report in Codecov by Sentry. |
kalebphipps
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good - I have only made some minor comments.
The one thing that should be changed before merging is to update all the Readthedocs pages for the tutorials and include Readthedocs pages for those tutorials that do not yet have such a page. I have included a longer comment about this - if you have any questions about what should be done feel free to ask and we can go through it together. Once this is done, I am happy to approve the merge :)
tests/data/paint_multiple_heliostats/AA39/heliostat-properties.json
Outdated
Show resolved
Hide resolved
tests/data/paint_multiple_heliostats/AA35/heliostat-properties.json
Outdated
Show resolved
Hide resolved
tests/data/paint_multiple_heliostats/AA31/heliostat-properties.json
Outdated
Show resolved
Hide resolved
|
I included all requested changes. |
|
It seems to me like our GitHub workflows are not yet configured for testing on GPU and also not configured for distributed testing. That is why the two lines in utils.py and alignment_optimizer.py are not yet covered by the tests. The tests do exist but they are not run. |
…tion/ARTIST into features/multi_heliostats
kalebphipps
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - I have made a few formatting changes and fixed a few typos locally - I will push them now and then merge
Updated structure for loading multiple heliostats at once.
This PR includes changes in the scenario structure and all classes and functions that are responsible for creating scenarios and those that are responsible for loading data from scenarios.
The
receiverclass was renamed to the more general termtarget areabecause we need to differentiate between receivers and calibration targets.The MPI parallelisation backend was removed and replaced with nccl or gloo depending on cpu or gpu use. This change was made because we currently only use DDP for parallelisation and MPI is not optimal for this.
Fixes #85
Type of change
Checklist: