8000 Switch `CompassConfigParser` to descend from `MpasConfigParser` by xylar · Pull Request #365 · MPAS-Dev/compass · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Switch CompassConfigParser to descend from MpasConfigParser #365

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 21, 2022
Merged

Switch CompassConfigParser to descend from MpasConfigParser #365

merged 3 commits into from
Apr 21, 2022

Conversation

xylar
Copy link
Collaborator
@xylar xylar commented Apr 20, 2022

This way, config parsing functionality can be shared between MPAS-Analysis and copass. See:
MPAS-Dev/MPAS-Tools#460
MPAS-Dev/MPAS-Analysis#873

The only functionality added to CompassConfigParser beyond what is in MpasConfigParser is to automatically convert config options in certain sections (paths, namelists, streams, and executables) from relative to absolute paths.

The documentation has been updated accordingly. It points to MPAS-Tools' documentation but also explains most of the functionality in relation to compass.

The new functionality requires mpas_tools >=0.14.0.

@xylar xylar self-assigned this Apr 20, 2022
@xylar xylar requested a review from matthewhoffman April 20, 2022 12:55
xylar added 3 commits April 20, 2022 14:56
@xylar
Update to mpas_tools v0.14.0
5251c5b
@xylar
Update docs
8053ac1
@xylar
Update config parser to decent from MpasConfigParser
20158f6 
This introduces new functionality like the `getexpression()` method
and allows shared functionality with MPAS-Analysis
@xylar xylar force-pushed the use_mpas_config_parser branch from 491eecc to 20158f6 Compare April 20, 2022 12:56
@xylar
Copy link
Collaborator Author
xylar commented Apr 20, 2022

Testing

I ran the ocean pr and MALI full_integration suites on Anvil, using the baseline I made for SCORPIO testing as the baseline here as well (so before the move to SCORPIO 1.3.2). Both test suites passed and are bit-for-bit.

@xylar
Copy link
Collaborator Author
xylar commented Apr 20, 2022

@matthewhoffman, could you give this a quick look when you have time? And maybe test it on Badger?

matthewhoffman
matthewhoffman approved these changes Apr 21, 2022
View reviewed changes
Copy link
Member
@matthewhoffman matthewhoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xylar , this looks great to move most of this module to a shared MPAS-Tools version. I did a MALI test and it works fine. My only question is if the the one remaining additional piece of functionality in the CompassConfigParser couldn't also be moved to the MpasConfigParser - even if MPAS-Analysis would never use it. It's not a big deal to have CompassConfigParser remain, but if it were possible to completely drop it, that might be a cleaner outcome to moving it to MPAS-Tools. I'm leaving that to your judgment if that makes sense or not and approving now.

@xylar
Copy link
Collaborator Author
xylar commented Apr 21, 2022

My only question is if the the one remaining additional piece of functionality in the CompassConfigParser couldn't also be moved to the MpasConfigParser - even if MPAS-Analysis would never use it. It's not a big deal to have CompassConfigParser remain, but if it were possible to completely drop it, that might be a cleaner outcome to moving it to MPAS-Tools. I'm leaving that to your judgment if that makes sense or not and approving now.

I thought about that but I'm concerned that it would be an annoying hidden feature if some other code used MpasConfigParser in the future. "Why is my config options that has nothing to do with a file path suddenly converted into a path?!?!" I had this when I tried to add a True/False config option to [executable] myself. I think it's much safer to leave it as a peculiarity specific to compass. Maybe the solution would be to deal with it in a function called explicitly rather than in this relatively hidden way. But I'm hesitant to add new, untested functionality at the moment...

@xylar xylar merged commit 58ab249 into MPAS-Dev:master Apr 21, 2022
@xylar xylar deleted the use_mpas_config_parser branch April 21, 2022 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewhoffman matthewhoffman matthewhoffman approved these changes

Assignees

@xylar xylar

Labels
clean-up framework
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xylar @matthewhoffman
0