Restructure package#203
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #203 +/- ##
==========================================
+ Coverage 96.06% 96.10% +0.04%
==========================================
Files 48 57 +9
Lines 3961 4008 +47
==========================================
+ Hits 3805 3852 +47
Misses 156 156 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ARTIST-Association/ARTIST into maintenance/restructure_package
|
As discussed in our last meeting, I adapted all imports to match the new package structure.
@MarleneBusch (@kalebphipps) The scripts in |
…ssociation/ARTIST into maintenance/restructure_package
for more information, see https://pre-commit.ci
…ARTIST-Association/ARTIST into maintenance/restructure_package
|
I have reviewed all changed files of this PR. I only added very minor changes (removed my system specific paths in the examples configs, which I forgot earlier, and one Overall this PR ist really good and I am amazed how much shorter and more concise the code got with this new structure and the revised imports. Thanks for also fixing docstring along the way. If its up to me this can be merged into the main now. I am wondering if we should include a short "How to import" in the contribution guidelines? I know that I havent fully understood all of the rules you applied in this PR for the imports (I only know it looks so much better) but I also know some of it was "Common sense" so I am not sure if we can even write down all the rules here. (I will also open an issue so we can consider renaming the motor pos optimization to aim point optimiaztion, which is not directly linked to this PR but just came back into my mind while reviewing this.) |
Description
This PR introduces a hopefully more reasonable and intuitive package structure:
I implemented the structure outlined above as is.
A couple of things need to be discussed before I can finalize this PR:
mean_loss_per_heliostat(now inartist/optimization/loss_functions.py) as well asazimuth_elevation_to_enuandconvert_wgs84_coordinates_to_local_enu(now inartist/geometry/coordinates.py). I am not super happy with this but did not find a better fit so @MarleneBusch please pay particular attention to functions that seem to be "misplaced" from your point of view.__init__.pyfiles of the package itself and its subpackages seemed to be overloaded which introduced new dependencies and caused unnecessary circular imports and long import times. A package's__init__.pyshould mark the directory as a package and define a clean public API but not wire together the entire project. I basically emptied all__init__.pyfiles for now. We should think about a clean public API for ARTIST, i.e., which stable, top-level concepts we want to expose explictly to users in the style offrom artist import Scenariovs.from artist.scenario.scenario import Scenario. Once we decided for a clean public API, we can update the tutorials and documentation accordingly.TYPE_CHECKINGfor type hints and a local imports for the method inartist/scenario/scenario.py. Are we fine with that?Type of change
Maintenance only, does not affect the functionality.
Checklist: