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

Clean input tap #746

Merged
merged 7 commits into from
Jul 20, 2023
Merged

Clean input tap #746

merged 7 commits into from
Jul 20, 2023

Conversation

jm-c
Copy link
Member

@jm-c jm-c commented Jul 13, 2023

What changes does this PR introduce?

  • Remove duplicated parameter files from input_tap and adjust prepare_run to use instead version from input_ad.
  • Remove un-tested (and un-maintained) additional dirs in exp. OpenAD (ad_singlelayer) and in lab_sea (code_tap.[adx.]noecco/).

What is the current behaviour?

  • Most (if not all) parameter files for Tapenade tests are identical to TAF corresponding tests (or OpenAD in case of OpenAD).
  • few untested (and not maintained) additional src and param files ; this make maintenance of the set of experiments more complicated.

What is the new behaviour

  • duplicated parameter files from */input_tap have been removed ; untested additional files also.

Does this PR introduce a breaking change?

no

Other information:

Suggested addition to tag-index

o verification:

  • remove duplicated parameter files from input_tap/ dir.

jm-c and others added 4 commits July 12, 2023 18:36
not tested, will not be maintained, better to remove them
instead of having a local copy in "input_tap", adjust "prepare_run" to use
version from "input_ad". This facilitates maintenance of many parameter files,
also make it obvious which file (if any) is different.
- content of code_ad_singlelayer/ & input_ad_singlelayer/ have not been
  maintained (not tested) and are lagging behind. This makes more tricky
  to maintain the remaining experiments and set-ups.
@jm-c jm-c requested review from Shreyas911 and mjlosch July 17, 2023 18:10
Copy link
Member

@mjlosch mjlosch left a comment

Choose a reason for hiding this comment

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

Looks good. Always good to remove duplcates. Note that I did not run this, because somehow my recipe to use Tapenade on my MacBook does not work anymore (incomplete notes?), and it takes me really long to figure out how to do this.

I noted a few points to consider:

  1. OpenAD/code_tap/grdchk_readparms.F is a copy of pkg/grdchk/grdchk_readparms.F and could be removed.
  2. These files are removed, but is it not clear to me why in this PR. Accidental? Something to check:
    • global_with_exf/input_ad/eedata.mth,
    • isomip/input_oad/eedata.mth,
    • tutorial_global_oce_biogeo/input_oad/eedata.mth
  3. Probably not related to this PR, but to be considered:
    halfpipe_streamice/code_tap/ctrl_map_gentim2d.F and halfpipe_streamice/code_tap/ctrl_map_gentim2d.F are copies of code_oad, but they have been put there because OpenAD could not handle the default code. I think Tapenade can and the experiments could be modeled after taf-versions, requiring fewer customised files.

The following are just notes, where some files could be simplified, but this really does not have anything to do with this PR:

  • remove to commented lines to make it more similar to pkg/cost/cost_test.F (from 3 to 1 difference): isomip/code_[ad,oad,tap]/cost_test.F
  • make more similar to pkg/cost/cost_tracer.F (add ifdef ALLOW_PTRACERS, remove unused variables thetaRef,ig, jg, blank lines, to get down to 2 differences from 8):
    tutorial_global_oce_biogeo/code_[ad,oad,tap]/cost_tracer.F

@jm-c
Copy link
Member Author

jm-c commented Jul 18, 2023

@mjlosch Thanks for all these good comments. In order:
(1) will remove this duplicated grdchk_readparms.F
(2) the eedata.mth is only used by testreport for multi-threaded tests (also served to decide which test "testreport -mth" will run). Since none of the TAF adjoint tests can run with more than 1 thread, these 3 eedata.mth are, at best, useless ; but also a little bit misleading, leaving the impression that these 3 set-up, at some point, have been able to run with multiple threads. So it's better to remove them, I think.
(3) I think it's a good point, but would prefer to leave changes to executable outside this PR (there is already a large enough number of removed files here) and add this as a new "bullet point" in your nice list above.

was identical to reference version from pkg/grdchk
@mjlosch
Copy link
Member

mjlosch commented Jul 19, 2023

LGTM

@jm-c
Copy link
Member Author

jm-c commented Jul 19, 2023

I think this PR is ready to be merged and, if no one wants to add things here, will merge it relatively soon, like tomorrow.

Copy link
Collaborator

@Shreyas911 Shreyas911 left a comment

Choose a reason for hiding this comment

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

Hi @jm-c, sorry for the delayed reply. I think these changes look good!

Best,
Shreyas

@jm-c
Copy link
Member Author

jm-c commented Jul 20, 2023

@Shreyas911 No problem here, and thanks for the feedback.
And just to repeat: This PR does not involve any changes to any MITgcm executable (mitgcmuv*) from any experiment, and no changes to any input files neither (parameter files or binary files). Just removing few unused files and using a single copy instead of multiple.

@jm-c
Copy link
Member Author

jm-c commented Jul 20, 2023

Going to merge this PR (very) soon

@jm-c jm-c merged commit 44c9a33 into MITgcm:master Jul 20, 2023
17 checks passed
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.

None yet

3 participants