Update build scripts #72
Conversation
Merging fork into branch
There was a problem hiding this comment.
I haven't gone through all the changes yet because I feel the job of reviewing will be a lot easier if you could make the following modifications:
- Remove all commented-out code.
- Drop mentions of your username in comments.
- Put common code from the large numbers of lines added to the
script/build*.pyscripts into a common location and import them. e.g., can'toutput_csvbe reused?
UM2N/generator/burgers_solver.py
Outdated
| # self.jacob_det = jacobian_det | ||
| # self.jacob_det = fd.project( | ||
| # jacobian_det, fd.FunctionSpace(self.mesh, "CG", 1) | ||
| # ) # ej321 - not needed? | ||
| # self.jacob = jacobian # ej321 - this is copied from mesh_generator.py |
There was a problem hiding this comment.
Please avoid commented out code - it makes diffs very hard to read. Also, old versions can always be deduced from the revision history.
script/build_helmholtz_square.py
Outdated
| type=str, | ||
| default="full", | ||
| help="scheme used to generate the dataset (pad/full))", | ||
| "--boundary_scheme", type=str, default="full", help="Boundary scheme (pad/full)" |
There was a problem hiding this comment.
Why did you change this? I feel like the proposed help message is less helpful?
There was a problem hiding this comment.
I moved the existing comment to the help string for clarity and to avoid duplication.
There was a problem hiding this comment.
It's difficult to review this PR because there's such a large number of lines changed and I'm not sufficiently familiar with them. Would it be possible to run some of the scripts as part of the CI testing framework? That way, we can at least be confident that they run without crashing.
| "swirl_params": self.swirl_params, | ||
| "t": self.t, # time step when solving burgers eq. | ||
| "idx": self.idx, # index number for picking params for burgers tracer. # noqa | ||
| "u": self.feature["uh"] if "uh" in self.feature else None, |
There was a problem hiding this comment.
This is equivalent to the following. If you could replicate this for the others, that'd be great.
| "u": self.feature["uh"] if "uh" in self.feature else None, | |
| "u": self.feature.get("uh"), |
| # i = 0 | ||
| # while i < n_samples: |
There was a problem hiding this comment.
Remove outdated comments here and elsewhere.
Update to build scripts. Key changes: