Skip to content
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

reworking and adjusting the output table function #116

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Dr3ven
Copy link
Collaborator

@Dr3ven Dr3ven commented Oct 12, 2023

I want to improve the output tables functions.
E.g. the old output tables function uses a dict inside a function to find the parameter description/name from the corresponding variable. I want to try to improve this to be more independent of the hard coded dict and descriptions.

Additionally, the tables are adjusted for the new rheologies that were added with #99.

@albert-de-montserrat
Copy link
Member

It would be nice to improve Tables.jl indeed. Taking a quick look, some suggestions:

  1. Get rid of all the global variables.
  2. There is a lot of code repetition in Phase2Dict. You can avoid all that type-based branching by defining small functions and using multiple dispatch.
  3. In general, all the functions are quite large and monolithic. Splitting them into smaller functions is probably a good idea when possible. Code will probably more clear and may also lead to code re-usability.

@albert-de-montserrat
Copy link
Member

Are you still planning to work on this? I am wondering whether it would makes sense to move this out to an external package, something like GeoParamsTables.jl, so we can move out some dependencies that are not really needed when one uses GeoParams in a solver or similar.

@Dr3ven
Copy link
Collaborator Author

Dr3ven commented Feb 13, 2024

Are you still planning to work on this? I am wondering whether it would makes sense to move this out to an external package, something like GeoParamsTables.jl, so we can move out some dependencies that are not really needed when one uses GeoParams in a solver or similar.

I do not have a lot of time at the moment, but I will finish this pull request once I have more time again.
Concerning the tables feature, the only output dependency that I use is Unidecode.jl as far as I know. So, I don't mind moving the tables to another package if that helps. I guess I would need to rethink how to code some stuff then, but just do as you think suits best for GeoParams.

@albert-de-montserrat
Copy link
Member

I think it make sense to move it out. It is not really needed inside GP. And I don't think it will change the current code structure, since its already a submodule, just need to update a few lines of code here and there.

@boriskaus
Copy link
Member

I'm fine with moving this to a separate package

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants