More flexible use of YAML files by the user of the library

nemocrys / pyelmer

A python interface to Elmer.

GNU General Public License v3.0

53 stars 15 forks source link

More flexible use of YAML files by the user of the library #3

Closed kdadzis closed 2 years ago

arvedes commented 3 years ago

I like the idea behind the example! How about renaming the new load_boundary function to load_data? Because that's what it actually does. Do you want to have this function for other classes (BodyForce, InitialCondition), too?

I'm not sure if the implementation with self.default_data_dir in Simulation class makes things a bit too complicated / less flexible. What do you think about leaving this out and ask the user to provide a file path instead? It wouldn't require much additional code in the setup but would allow, e.g., to specify boundaries in different files and gives more flexibility regarding the file naming.

Besides that: Changing len(simulation.default_data_dir)>0 to simulation.default_data_dir != "" would increase readability :)

We should remove the .pyc file with the next commit! It's a leftover from when I didn't have a gitignore yet...

Shall I continue with this or do you want to go ahead?

kdadzis commented 3 years ago

A few comments:

Renaming the function to load_data AND adding such a function to all classes is a good idea!
Main motivation for default_data_dir in Simulation class was to keep the code (much) shorter if someone wants to load own data for many solvers, boundaries, etc. The possibility to specify own path in the other classes is of course still there! Same is true for the default data directory in the library itself (although I would be rather careful with this one since the user might not even be aware that is is being used!)
I agree with != ""
Yes, .gitignore should include .pyc

Please, go ahead with merging these changes.

arvedes commented 3 years ago

I'm not convinced by the default data dir, it's not flexible enough. It's easy enough to save these couple of lines in other ways. I'll revise and merge the rest!

kdadzis commented 3 years ago

Well, in my example I need to set the path to load all solver/material/boundary data only once :

elmer.load_simulation("axi-symmetric_steady", None, "yaml")

If there is another, more flexible solution which does not require to add a path to every load_*() call - problem solved!

arvedes commented 3 years ago

I'll make a suggestion!

arvedes commented 3 years ago

The interface for the yaml-files will be reworked as sketched in #6. I'll integrate this PR / the suggested workflow together with the modifications described there.

arvedes commented 2 years ago

I implemented it in a slightly different way, it's on the master branch now. Can you have a look at this @kdadzis ? I also update the crystal growth example, I think it's a good starting point.

kdadzis commented 2 years ago

The current version of pyelmer & examples look good with respect to the issues discussed above. The pull request is probably not needed any more and can be closed.