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

use Atlas structures with an IFS-like variable batching #60

Merged
merged 17 commits into from
Feb 7, 2024
Merged

use Atlas structures with an IFS-like variable batching #60

merged 17 commits into from
Feb 7, 2024

Conversation

sbrdar
Copy link
Collaborator

@sbrdar sbrdar commented Sep 28, 2023

Using Atlas structures in CLOUDSC via configuration flag "--with-atlas"

  1. variable batching (as in GML fields of IFS) is enabled by default via atlas::MultiField
  2. no batching is possible by setting the environment variable CLOUDSC_ATLAS_MULTIFIELD to 0
  • with Willem Deconinck
  • thanks to Balthasar Reuter

@sbrdar
using two MultiField-s to group variables under one block; still rely…
019d02a 
…ing on FieldSet for other datatype/dimension/level_number variables; using Field lookup by name
@sbrdar
working out numbering for input variables in multifield
31cbe4d
@sbrdar
using Field lookup be idx (more performance) whilst using MultiField
ee340be
@sbrdar
clean up memory leaks of Atlas (with Willem)
961bef8
@sbrdar
add Atlas tracing (with Willem)
dc9b919
@sbrdar
ensure OMP stays activates after Atlas and its dependencies
4349235
@sbrdar
cleanup
acc7299
@sbrdar
fixed for single precision in Atlas structures
4f56faa
@sbrdar
support single precision in expand_atlas_mod
ea6d374
@sbrdar
make atlas batching with MultiField work for SerialBox
3199964
@sbrdar
ensure correct datatype in MultiField for the single precision
69501b7
@sbrdar
another attempt at fixing SerialBox for Atlas structure
ee502b5
@reuterbal reuterbal changed the base branch from main to develop September 28, 2023 09:22
@reuterbal
Copy link
Collaborator

With #54 merged, this needs a rebase. @sbrdar Can you take care of that so we can bring this variant in as well?

@sbrdar
Copy link
Collaborator Author

sbrdar commented Jan 23, 2024

With #54 merged, this needs a rebase. @sbrdar Can you take care of that so we can bring this variant in as well?

I have merged now 'develop' into 'nab_atlas_MultiField'.

@sbrdar sbrdar requested a review from reuterbal January 24, 2024 09:12
reuterbal
reuterbal requested changes Feb 7, 2024
View reviewed changes
Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

Thanks and apologies for the review latency. This looks great!

I also like the introduction of atlas_trace.

I've flagged a few places with commented code leftovers, which should be removed.

Second request is to add a brief (!) description to the README (just an entry under "prototypes available").

bundle.yml
@@ -61,7 +61,7 @@ projects :

- atlas :
git : https://github.com/ecmwf/atlas
version : 0.34.0
version : feature/MultiField
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just out of curiosity: Is there a timeline for including this feature in a release?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Willem is going to include the feature in the release very soon.

src/cloudsc_fortran_atlas/cloudsc_global_atlas_state_mod.F90 Outdated
Comment on lines 135 to 137
! CALL FSET%UPDATE_DEVICE(1,IBLK)
! field = fset%field(1)
! call field%update_device()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Debug leftover?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. The lines are removed.

src/cloudsc_fortran_atlas/cloudsc_global_atlas_state_mod.F90 Outdated
OUT_MFIELD_CONFIG(IVAR) = ATLAS_CONFIG()
CALL OUT_MFIELD_CONFIG(IVAR)%SET("name", TRIM(OUT_VAR_NAMES(IVAR)))
CALL OUT_MFIELD_CONFIG(IVAR)%SET("levels", SELF%KLEV+1)
! CALL FSET%ADD(FSPACE%CREATE_FIELD(NAME=TRIM(OUT_VAR_NAMES(IVAR)), KIND=ATLAS_REAL(JPRB)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

debug leftover?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. The lines is removed.

src/cloudsc_fortran_atlas/validate_atlas_mod.F90 Outdated
NPROMA = FIELD%SHAPE(1)

ZMINVAL(1) = +HUGE(ZMINVAL(1))
ZMAX_VAL_ERR(1) = -HUGE(ZMAX_VAL_ERR(1))
ZMAX_VAL_ERR(2) = 0.0_JPRB
ZSUM_ERR_ABS(:) = 0.0_JPRB

!CALL INPUT_INITIALIZE(NAME='reference')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Debug leftover?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. The line is removed.

src/cloudsc_fortran_atlas/validate_atlas_mod.F90 Outdated
@@ -160,6 +160,7 @@ SUBROUTINE VALIDATEVAR_ATLAS(FSET, NAME, NLON, NGPTOTG, STATE_VAR)
PRINT *, "FIELD RANK NOT SUPPORTED"
CALL EXIT(1)
ENDIF
!CALL INPUT_FINALIZE()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Debug leftover?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True. I have removed the line.

src/cloudsc_fortran_atlas/cloudsc_global_atlas_state_mod.F90
Comment on lines +228 to +234
LMULTIFIELD = .TRUE.
CALL GET_ENVIRONMENT_VARIABLE("CLOUDSC_ATLAS_MULTIFIELD",CENV,CENV_LEN)
IF (CENV_LEN > 0 ) THEN
IF( TRIM(CENV) == "0" .OR. TRIM(CENV) == "OFF" .OR. TRIM(CENV) == "FALSE" ) THEN
LMULTIFIELD = .FALSE.
ENDIF
ENDIF
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed that we don't have a description of this variant in the README. Could you add just 2-3 sentences that explain what this variant is (e.g., "uses Atlas as storage backend for field data") and that two choices for the backend exist Multifield/FieldSet, which can be switched using that environment variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! Added 2-3 sentences to README. Thanks for the review!

reuterbal
reuterbal approved these changes Feb 7, 2024
View reviewed changes
Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

Many thanks for making these changes! Good to go!

@reuterbal reuterbal merged commit 60825ea into develop Feb 7, 2024
18 checks passed
@reuterbal reuterbal deleted the nab_atlas_MultiField branch February 7, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reuterbal reuterbal reuterbal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sbrdar @reuterbal