Refactor 4C random fields#176
Conversation
Fix will follow in PR queens-py#176
Fix will follow in PR queens-py#176
leahaeusel
left a comment
There was a problem hiding this comment.
Just one thought upfront, without having read all the changes thoroughly:
There was a problem hiding this comment.
Is there a particular reason why you did not follow the
└── src/
├── queens/ # contains QUEENS stuff
├── queens_4C/ # Contains all the 4C related stuff
└── queens_XXX/ # Contains all the QUEENS stuff related to a project XXX
layout which you proposed in #174?
There was a problem hiding this comment.
No particular reason, I was unsure what's better. That's why I asked for some feedback.
Honestly, I can live with both. What's your preference?
There was a problem hiding this comment.
Maybe relevant for the decision process
First proposition
from queens_4C.some_module import SomeClass- Each project has its own subpackage
- Redundant
queens_
Second proposition
from queens_interfaces.fourc.some_module import SomeClass- Each project has its own subpackage, but is grouped with all other interfaces
- no redundant name but is longer
There was a problem hiding this comment.
I think I have slight preferences for the second option: From a modularity point of view the existence of queens_interfaces to me implies that I as a developer should be able to rely on a well-defined interface to the actual solvers when developing core queens code, while the existence of queens_4C or queens_fenics sub packages would not necessarily guarantee me this.. I mean, obviously in neither of the cases these assumptions need to be true, but that's why I prefer the second option from a psychological point of view --- not sure if this statement now made sense to anyone, but still 😄
And as for the drawback of being longer: It would still be possible to shorten it, e.g., if queens_interfaces.fourc would be needed more often inside the code, one could replace it through import queens_interfaces.fourc as queens_fourc or something similar. So I would vote option 2) but I also don't have a strong opinion :)
There was a problem hiding this comment.
I have a slight preference towards the first layout because it reduces nestedness, but I'm also very much open to other arguments 🤷♀️
There was a problem hiding this comment.
From a dependency point of view, the first layout also has the advantage that each user would only have to install the queens_* packages that they actually need and thus only the dependencies that come with these particular packages
There was a problem hiding this comment.
From a dependency point of view, the first layout also has the advantage that each user would only have to install the queens_* packages that they actually need and thus only the dependencies that come with these particular packages
Small correction. When you install Queens, all the packages (queens_4C, queens_XXX) are installed, but not their dependencies (only those you specified, so pip install .[fourc] or for both pip install .[fourc,XXX]).
This means the packages are installed, but are not guaranteed to work as dependencies might be missing.
It's possible to separate them, but that is complicated to do (therefore not common). Instead, what is commonly done is to raise an error if a package is missing, see for example in dask. So we could do something like this within a module in the queens 4C subpackage:
try:
import fourcipp
except ImportError as exc:
if exc.msg == "No module named 'fourcipp'":
raise ImportError("Package fourcipp is not installed, install via <better message here>") from exc
raise excThere was a problem hiding this comment.
Thanks for the clarification! I was assuming we would separate the packages in the future, but if this is uncommon and complicated to do, raising an informative error, as you suggested, seems to be a good workaround 👍
There was a problem hiding this comment.
I added a similar message to the 4C random fields module
There was a problem hiding this comment.
I like that we separate the dependencies for the different packages.
Would this approach also work for large dependencies, such as TensorFlow, throughout the rest of the code (the main QUEENS package)?
Fix will follow in PR queens-py#176
facdff3 to
314d865
Compare
314d865 to
e965e92
Compare
e965e92 to
fb08a88
Compare
fb08a88 to
7fc69f9
Compare
55e86f8 to
b8ab9a6
Compare
|
@maxdinkel @sbrandstaeter @leahaeusel could we get this PR before it diverges too much? |
sbrandstaeter
left a comment
There was a problem hiding this comment.
Many thanks to @gilrrei for implementing these important changes.
There is quite a lot going on in this PR, including infrastructure changes and new features. The refactoring of the example simulator functions was missing in the PR descirptions but that's easy to fix. In general, it might be easier to review and to document things if the steps were separated into individual PRs but it also increases overhead so I think we are fine for now.
I have reviewed in great detail, with a focus on naming and documentation, which naturally involves some personal opinions. Goal was however to set this really important feature up for a robust and long lifetime.
| @@ -0,0 +1,3 @@ | |||
| # Example simulator functions | |||
|
|
|||
| This package contains a wide range of functions for benchmarking. | |||
There was a problem hiding this comment.
| This package contains a wide range of functions for benchmarking. | |
| This package provides a suite of analytical functions for method testing, validation and benchmarking. |
I was trying to make it a touch more descriptive for people new to the field.
| Modules for interfacing the for multiphysics code 4C ( | ||
| https://github.com/4C-multiphysics/4C) |
There was a problem hiding this comment.
| Modules for interfacing the for multiphysics code 4C ( | |
| https://github.com/4C-multiphysics/4C) | |
| Modules for interfacing with the 4C multiphysics code ( | |
| https://github.com/4C-multiphysics/4C) |
| "The required packages to construct random fields in QUEENS for 4C are not installed." | ||
| " Please install them via \n pip install -e .[fourc]" |
There was a problem hiding this comment.
| "The required packages to construct random fields in QUEENS for 4C are not installed." | |
| " Please install them via \n pip install -e .[fourc]" | |
| "The packages required to interface with 4C are not installed." | |
| " Please install them via \n pip install -e .[fourc]" |
In this way, the error message can be reused if other functionality for the interface is introduced in the future.
The user should know what he was using the interface for anyway.
There was a problem hiding this comment.
I like that we separate the dependencies for the different packages.
Would this approach also work for large dependencies, such as TensorFlow, throughout the rest of the code (the main QUEENS package)?
| @@ -0,0 +1,10 @@ | |||
| # 4C | |||
|
|
|||
| This package contains drivers and utilities realted to the for multiphysics code [4C](https://github.com/4C-multiphysics/4C). We like them and we work closely with them. | |||
There was a problem hiding this comment.
| This package contains drivers and utilities realted to the for multiphysics code [4C](https://github.com/4C-multiphysics/4C). We like them and we work closely with them. | |
| This package contains modules (drivers and utilities) that interface QUEENS with the multiphysics code [4C](https://github.com/4C-multiphysics/4C). We like them and we work closely with them. |
| This package contains drivers and utilities realted to the for multiphysics code [4C](https://github.com/4C-multiphysics/4C). We like them and we work closely with them. | ||
|
|
||
| ## Material random field interface | ||
| In order to create random material field in combintation with 4C, we require the package [fourcipp](https://github.com/4C-multiphysics/fourcipp). Therefore install QUEENS via (in the QUEENS main directory): |
There was a problem hiding this comment.
| In order to create random material field in combintation with 4C, we require the package [fourcipp](https://github.com/4C-multiphysics/fourcipp). Therefore install QUEENS via (in the QUEENS main directory): | |
| To create random material fields in combination with 4C, we require the package [fourcipp](https://github.com/4C-multiphysics/fourcipp). Therefore, install QUEENS via (in the QUEENS main directory): |
| Path(template_path).write_text(template_data) | ||
|
|
||
|
|
||
| def extract_by_material_id(material_id): |
There was a problem hiding this comment.
There might be a better name for this function.
| dict: with parameter names and element locations | ||
| """ | ||
| fourc_input = FourCInput.from_4C_yaml(path_to_template) | ||
| extract_elements(fourc_input, elements_section, extracting_condition) |
There was a problem hiding this comment.
| extract_elements(fourc_input, elements_section, extracting_condition) |
Can this be deleted?
| """QUEENS interfaces. | ||
|
|
||
| Modules for preprocessing geometry from external simulation software. | ||
| QUEENS interfaces for different packages. |
There was a problem hiding this comment.
| QUEENS interfaces for different packages. | |
| Interfaces for coupling QUEENS with solvers. |
sbrandstaeter
left a comment
There was a problem hiding this comment.
Due to this PR blocking #187, we decided to move forward here and fix my comments regarding naming etc. in a follow-up.
Description and Context:
This PR refactors 4C related stuff:
pip install -e .[dev,fourc](only needed if you want to work with random fields, not with 4C)from_fileapproach from 4C is used.I would appreciate some feedback on this before finishing the PR :)
Related Issues and Pull Requests
Interested Parties