Skip to content

[PULL REQUEST] Adjoint run skeleton#71

Closed
TerribleNews wants to merge 14 commits intogeoschem:devfrom
TerribleNews:add_ems_adj
Closed

[PULL REQUEST] Adjoint run skeleton#71
TerribleNews wants to merge 14 commits intogeoschem:devfrom
TerribleNews:add_ems_adj

Conversation

@TerribleNews
Copy link

I am submitting this pull request mostly to get the discussion started. The code should not change the standard GCHP at all, but allows for the flags -DADJOINT and -DREVERSE_OPERATORS to be defined at cmake time which enable runtime flags for reversing the timestep and computing adjoint variables (i.e., a single adjoint-enabled binary is compiled which can be run in forward and reverse mode for adjoint operations). The adjoint calculation code is still a work-in-progress, but a trying to keep up with the changing structure of the git repositories has become very difficult so I'd like to get the working reverse run code into the geoschem/GCHP repo ASAP. I will also submit pull requests for the submodules MAPL, HEMCO, geos-chem and FVdycoreCubed_GridComp to go along with this pull request since this code only compiles with the changes I made to those submodules.

@LiamBindle
Copy link
Contributor

Cool, thanks Colin! I'll add this to the 13.1 milestone.

Sorry the GCHP restructuring was a burden on your end. Now that the restructuring is complete, things should be a lot more stable moving forward.

I see there are a couple conflicts. Are those trivial things we can fix at merge time? (I've also seen GitHub identify conflicts that don't actually exist).

@LiamBindle LiamBindle added this to the 13.1.0 milestone Jan 5, 2021
@TerribleNews
Copy link
Author

The only conflicting "files" I see are the submodules ESMA_cmake, geos-chem, and HEMCO. I have learned while trying to get this pull request ready that there is no real way to merge submodules and so I don't know how to make sure that the repos stay in sync with the pull. In any case, I think you can ignore those and point the merged branch at the correct submodule commits once they've been merged into their respective repos. Let me know if that doesn't make sense and I'll try to clarify.

@LiamBindle
Copy link
Contributor

LiamBindle commented Jan 6, 2021

Oh, I see. Yeah that's easy to do with the merge. Thanks!

@yantosca yantosca changed the title Adjoint run skeleton [PULL REQUEST] Adjoint run skeleton Jan 13, 2021
@yantosca yantosca added the never stale Never label this issue as stale label Jan 13, 2021
@msulprizio msulprizio added category: Feature Request New feature or request and removed never stale Never label this issue as stale labels Mar 3, 2021
@lizziel lizziel changed the base branch from main to dev April 6, 2021 21:17
Comment on lines +697 to +701
if (.not. firstRun) THEN
call fv_computeMassFluxes(UCr8, VCr8, PLEr8, &
MFXr8, MFYr8, CXr8, CYr8, dt)
endif
firstRun = .false.
Copy link
Contributor

Choose a reason for hiding this comment

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

This change here is causing differences in the GCHP benchmark run. Could you explain why you put this here? Is it only for adjoint?

Copy link
Author

Choose a reason for hiding this comment

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

This has to do with the reversal of the high-level operators; I have put it in an #ifdef ADJOINT block.

@lizziel
Copy link
Contributor

lizziel commented Apr 7, 2021

Also, don't worry about the conflicts. I am going to bring the file changes for this PR in manually to avoid the submodule updates.

@TerribleNews
Copy link
Author

Also, don't worry about the conflicts. I am going to bring the file changes for this PR in manually to avoid the submodule updates.

Okay, thank you! Is there a "correct" or at least better way to deal with submodule updates?

@lizziel
Copy link
Contributor

lizziel commented Apr 7, 2021

Thanks for the quick turn-around! We are still figuring this out, but for now think the best way is to have super-project PRs include file changes but no changes to submodules. Then in the PR conversation list out the submodules that need to be updated with links to the individual PRs for them. Does that make sense?

@lizziel
Copy link
Contributor

lizziel commented Apr 7, 2021

I have pushed the file updates manually to dev along with submodule updates for HEMCO, GEOS-Chem, and FVdycoreCubed_GridComp. I could not find a PR for ESMA_cmake. Did you have updates you need to make there? I am also holding off on the MAPL submodule update. We can move discussion of that to the MAPL PR.

@TerribleNews
Copy link
Author

TerribleNews commented Apr 8, 2021

Then in the PR conversation list out the submodules that need to be updated with links to the individual PRs for them.

So sort of like what I did but backwards?

I have pushed the file updates manually to dev along with submodule updates for HEMCO, GEOS-Chem, and FVdycoreCubed_GridComp.

Wonderful! Thank you!

I could not find a PR for ESMA_cmake. Did you have updates you need to make there?

Sorry, I've created that PR now. geoschem/ESMA_cmake#4

I am also holding off on the MAPL submodule update. We can move discussion of that to the MAPL PR.

Yes, thank you. I responded on there and will get back to you soon.

@lizziel
Copy link
Contributor

lizziel commented Jun 11, 2021

Hi @TerribleNews, all of the updates except for MAPL are now merged in for 13.1. Would you like to keep this PR open to continue discussion of the MAPL updates that will eventually go in, or can we close and move that discussion to MAPL github?

@TerribleNews
Copy link
Author

Hi @TerribleNews, all of the updates except for MAPL are now merged in for 13.1. Would you like to keep this PR open to continue discussion of the MAPL updates that will eventually go in, or can we close and move that discussion to MAPL github?

That's a good question. My plan was to send duplicate pull requests here and to GMAO, since there is likely to be a pretty long lag time for the GMAO changes to make it here. I am still trying to sort out what is in that pull request as I have had some difficulty getting things to build with the dev branch, and also the cluster I was working on was down for 2 weeks for maintenance. Does the duplicate PRs plan make sense or are you expecting to only get MAPL changes via GMAO?

@lizziel
Copy link
Contributor

lizziel commented Jun 11, 2021

We can put updates in without going through GMAO for the short-term, to be superceded by what GMAO brings in. They will want to implement them their own way and in such a way that does not impact GEOS. The PR you do with them will give them a starting point - they may want to iterate with you or just see the update and do it themselves.

So to answer your question, there should be two PRs but they may not be identical. GMAO is on MAPL 2.7.1 while we are still on 2.6.3.

@lizziel
Copy link
Contributor

lizziel commented Jun 17, 2021

I will close this PR. The remaining work to be done is for MAPL which has open PR geoschem/MAPL#4.

@lizziel lizziel closed this Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: Feature Request New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants