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.

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

[PULL REQUEST] Forward and reverse models compile and run #67

Merged
merged 5 commits into from
Apr 7, 2021
Merged

[PULL REQUEST] Forward and reverse models compile and run #67

merged 5 commits into from
Apr 7, 2021

Conversation

TerribleNews
Copy link

Companion pull request to geoschem/GCHP#71

I needed to add HEMCO code to calculate emissions sensitivities, apply emissions scaling factors, and output adjoint diagnostics.

@yantosca yantosca changed the title Forward and reverse models compile and run [PULL REQUEST] Forward and reverse models compile and run 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
@msulprizio msulprizio added this to the 3.1.0 milestone Mar 3, 2021
@lizziel
Copy link
Contributor

lizziel commented Mar 22, 2021

Hi @TerribleNews, thanks for these updates. I am bringing these in for the next release of HEMCO. I have similar questions as for the GEOS-Chem companion PR.

  1. Are all of the prints currently included meant for inclusion in the model (i.e. not temporary debugging code)?
  2. Have you checked that the updates will not break GC-Classic due to use of MAPL code (I'm looking at the all the uses of MAPL_am_I_Root)?

It's been on the to do list for a while to improve the error handling and prints in HEMCO, especially when using MPI. If some or all of your prints were for temporary debugging we might still want to bring them in, but maybe with tweaking if they are MAPL-dependent.

@TerribleNews
Copy link
Author

Hi @TerribleNews, thanks for these updates. I am bringing these in for the next release of HEMCO. I have similar questions as for the GEOS-Chem companion PR.

1. Are all of the prints currently included meant for inclusion in the model (i.e. not temporary debugging code)?

It's been on the to do list for a while to improve the error handling and prints in HEMCO, especially when using MPI. If some or all of your prints were for temporary debugging we might still want to bring them in, but maybe with tweaking if they are MAPL-dependent.

Okay, I'll look into this and get back to you. What is the plan for debug prints?

2. Have you checked that the updates will not break GC-Classic due to use of MAPL code (I'm looking at the all the uses of MAPL_am_I_Root)?

Oh, that's a good question. I've never build GEOS-Chem classic from the GCHP repo. I will do a quick visual check, but it might take me a while to get to testing if I can build (and run?) classic.

@TerribleNews
Copy link
Author

I think I've covered most of these now. For now I just eliminated the debug printing altogether. It shouldn't be hard to add back in once there's more infrastructure for handling MAPL/non-MAPL cases.

@lizziel
Copy link
Contributor

lizziel commented Apr 5, 2021

Thanks @TerribleNews! I'll bring this in this week and let you know if I have any questions.

@lizziel lizziel changed the base branch from main to dev April 6, 2021 16:15
lizziel
lizziel reviewed Apr 6, 2021
View reviewed changes
src/Interfaces/Shared/hco_interface_common.F90 Outdated
@@ -330,5 +333,511 @@ SUBROUTINE GetHcoDiagn ( HcoState, ExtState, DiagnName, &
RC = HCO_SUCCESS

END SUBROUTINE GetHcoDiagn

#ifdef FALSE ! the adjoint-relevant code in here has been moved to
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intend to leave this as FALSE?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I missed that when I cleaned up the other ifdef FALSEs. This whole subroutine was moved to hco_calc_mod but I left it there as insurance in case that didn't work and then forgot to delete it after I verified the new code. I have commited a change removing both these blocks.

lizziel
lizziel reviewed Apr 6, 2021
View reviewed changes
src/Interfaces/Shared/hco_interface_common.F90 Outdated
@@ -28,6 +28,9 @@ MODULE HCO_Interface_Common
PUBLIC :: SetHcoTime
PUBLIC :: GetHcoVal
PUBLIC :: GetHcoDiagn
#ifdef FALSE
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above. Should this be ADJOINT?

lizziel
lizziel reviewed Apr 7, 2021
View reviewed changes
src/Core/hcoio_write_esmf_mod.F90 Outdated
Comment on lines 161 to 164
IF (MAPL_am_I_Root()) THEN
IF ( RC == HCO_SUCCESS .and. FLAG == HCO_SUCCESS ) WRITE(*,*) "Got it! Name: ", TRIM(ThisDiagn%cName)
IF ( RC /= HCO_SUCCESS) WRITE(*,*) "Fail! RC: ", RC, " Flag: ", FLAG
ENDIF
Copy link
Contributor

Choose a reason for hiding this comment

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

In my GCHP test this is printing lines every timestep.

Copy link
Author

Choose a reason for hiding this comment

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

Those wound up being pretty key debug messages. I don't know if we'll need them again, but probably. Can I comment them out but keep them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. We want to overhaul the HEMCO debug prints and if this stays in but commented out it will be included in that update.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, done!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@lizziel lizziel merged commit 9d5dc5a into geoschem:dev Apr 7, 2021
@msulprizio msulprizio modified the milestones: 3.1.0, 3.0.0 Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizziel lizziel lizziel left review comments

Assignees

@lizziel lizziel

Labels
category: Feature Request New feature or request
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@TerribleNews @lizziel @msulprizio @yantosca