-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: Add support for non_parametric optimization #100
Conversation
50c09f3
to
2f6d909
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #100 +/- ##
==========================================
+ Coverage 83.62% 85.55% +1.92%
==========================================
Files 44 44
Lines 2589 2630 +41
==========================================
+ Hits 2165 2250 +85
+ Misses 424 380 -44 ☔ View full report in Codecov by Sentry. |
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.
It looks fine. I just have a few comments.
I'm also wondering if a run_parametric
and run_non_parametric
could be a separated dataclass
/pydantic class
with fields and validators for avoiding having all the functions classified as internally used (_
)
""" | ||
geometry = get_object_from_identifiable(geometry, self._client._geometry_directory) | ||
objective = _build_objective(minimize, maximize) | ||
optimization_parameters = { |
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.
since optimization parameters
is used both in run_parametric
and run_non_parametric
with small differences, it might deserve to have a function or class for its construction, according to parametric or non-parametric. Maye it would save some a few lines of code too 😄
for _ in range(n_iters): | ||
logger.debug("Running iteration") | ||
progress_bar.set_description("Running iteration") | ||
trial_run = optimization._run_iteration({}) |
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.
This {}
arg could really use a validation with a raise InvalidArguments
optimization: Identifiable[Optimization], | ||
geometry: Identifiable[Geometry], | ||
geometry_parameters: Dict, | ||
def _run_iteration( |
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.
nit: same method name for both optim and optimtrialrun objects, but with different args..
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.
Generally nice but data/optimizations.py
is hard to understand
self, | ||
geometry: Identifiable[Geometry], | ||
bounding_boxes: List[List[float]], | ||
symmetries: List[Literal["x", "y", "z", "X", "Y", "Z"]], |
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.
I think the value of typing for both upper and lower case is not worth it compared to "Hugh ? Why am I seeing double ?"
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.
Since I could not wrap my head around the interaction of the different components I made this:
sequenceDiagram
participant User
participant OptDir as OptimizationDirectory
participant TrialDir as OptimizationTrialRunDirectory
User ->> OptDir: run_parametric
activate User
activate OptDir
create participant Opti as Optimization
OptDir -x Opti: creates
loop Until optimization complete
OptDir->>Opti: _run_iteration
activate Opti
Opti->>TrialDir: _run_iteration
activate TrialDir
create participant Trial as OptimizationTrialRun
TrialDir-xTrial: Creates
activate Trial
TrialDir-->>Opti: returns OptimizationTrialRun
deactivate TrialDir
Opti-->>OptDir: returns OptimizationTrialRun
deactivate Opti
OptDir->>Trial: Get iteration results
deactivate Trial
end
deactivate OptDir
OptDir-->>User: result (List[Dict])
deactivate User
Conclusions:
- Some objects are not meant to be handled by end users (OptimizationTrialRun and OptimizationTrialRunDirectory so should be prefixed by "_")
- The relationships between objects must be better defined in the code, even in the definition order, define the children first:
OptimizationTrialRun
thenOptimizationTrialRunDirectory
thenOptimization
thenOptimizationDirectory
- Maybe
_run_iteration
should only be defined on theOptimization
? - I don't understand what
OptimizationTrialRunDirectory.get
could be used for
2f6d909
to
903d6a7
Compare
903d6a7
to
66151f9
Compare
66151f9
to
c93de56
Compare
No description provided.