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

Refactor gridspacedesc.yml so all the subset info is standard and built in, and not user supplied #33

Open
eldond opened this issue Jan 18, 2024 · 6 comments
Assignees

Comments

@eldond
Copy link
Collaborator

eldond commented Jan 18, 2024

No description provided.

@eldond
Copy link
Collaborator Author

eldond commented Jan 18, 2024

probably it should be in GGDUtils

@anchal-physics
Copy link
Collaborator

How about I move default gridspecdesc.yml to src and make it a default argument. That way we can easily change defaults in future and it will leave the possibility open of supplying user-specified description.

@eldond
Copy link
Collaborator Author

eldond commented Jan 23, 2024

That would solve the immediate issue. I wonder if maybe the subset definitions should be in a separate file, which would make them easier to find (if the file were subsets.yml) and also I think the use case for changing subsets is different from changing the other setup that's in that file.

But even if you don't want to split, what you say seems fine.

anchal-physics added a commit to ProjectTorreyPines/SOLPS2imas.jl that referenced this issue Feb 14, 2024
Argument gsdesc of solps2imas function has been made a keyword
argument now with default value given as
src/default_grid_space_description.yml. Now, the user can skip
providing this file and rely on code defaults for most of the use
cases. This commit resovles
ProjectTorreyPines/SOLPS2ctrl.jl#33

Note, that SD4SOLPS.jl requires some changes now.
anchal-physics added a commit that referenced this issue Feb 14, 2024
PR ProjectTorreyPines/SOLPS2imas.jl#24
will change gsdesc to a keyword argument with default file. This
commit changes all use cases of solp2imas to use the default
gsdesc value instead of supplying a file. This is in support of
closing
#33
@anchal-physics
Copy link
Collaborator

I did not go ahead with file splitting, partially because I could not understand what you meant. Creating 25 extra files for subset just to hold tiny amounts of meta data seemed too much to me, but maybe I misunderstood what you meant. I've opened two PRs that will resolve this issue as per the suggested changes above:

ProjectTorreyPines/SOLPS2imas.jl#24
#41

anchal-physics added a commit that referenced this issue Feb 14, 2024
PR ProjectTorreyPines/SOLPS2imas.jl#24
will change gsdesc to a keyword argument with default file. This
commit changes all use cases of solp2imas to use the default
gsdesc value instead of supplying a file. This is in support of
closing
#33
@eldond
Copy link
Collaborator Author

eldond commented Feb 14, 2024

@anchal-physics No, not 25 files. Just two. One for the space and index stuff, and one for all of the subsets. Well, actually three might be better:

  1. space and index (because I think it's allowed to load one IDS file with a bunch of ggd_idx instances for holding different things)
    • The reason this is a different file is because there's a use case for the end user to customize it, and it's very short and therefore edits are tractable (if the subsets are taken out).
  2. standard subsets (all the ones from the docs)
    • The reason this is a different file is that its source is all just GGD documentation
    • This file would be updated if the makers of GGD changed the standard indices
  3. extended subsets (all the ones we added with negative indices)
    • The reason this is a different file is that its source is all just us making stuff up
    • This file would be updated if we made up new extended subsets

@anchal-physics
Copy link
Collaborator

Aah, I got it now. I'll request new PRs in the future bringing this feature. We can leave this issue open to signify remaining work.

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

No branches or pull requests

2 participants