Data Generation Separation for Gridsearch #35
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mergging PDE changes
…first step result
…_params from now on
There was a problem hiding this comment.
Hey Yash, thanks for separating out data generation! First round of review:
- This is too much stuff not related to separating data generation. Let's work on just PRing a single piece of functionality.
- It's helpful to describe the API changes in the PR comment.
- ...as well as some simple command that demonstrates the change. Have you tried this with a
MockExperiment? - I assume
data_prepis its own experiment step, right? Maybe a better approach would have been better accepting merelysim_paramsand lettinggridsearchdecide when to generate new simulation data. Let's chat about approach tomorrow.
| @@ -16,6 +16,15 @@ | |||
| logger = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| class PDEData(dict): | |||
There was a problem hiding this comment.
Suggested change
| class PDEData(dict): | |
| class PDEData(TypedDict): |
(requires importing TypedDict from typing
| dt=time_args[0], | ||
| t_end=time_args[1], | ||
| ) | ||
| rel_noise = sim_params["rel_noise"] |
There was a problem hiding this comment.
Why are rel_noise and t_end given such special treatment?
| len(spatial_grid), | ||
| ] | ||
| if grid_params is not None: | ||
| start_time = process_time() |
There was a problem hiding this comment.
the you can move calls to process_time() outside the conditional so we can remove redundant lines of code
Comment on lines
+156
to
+157
| t_end = combo[0] | ||
| rel_noise = combo[1] |
There was a problem hiding this comment.
What about other simulation parameters?
| def run( | ||
| seed: float, | ||
| data: dict, |
There was a problem hiding this comment.
Need a more complete type information for data
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces data generation separation for gridsearch experiments.