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

Adds support for opt_archive parameter in pp_ser.py Python preprocessor #14

Merged
merged 4 commits into from
May 30, 2017
Merged

Adds support for opt_archive parameter in pp_ser.py Python preprocessor #14

merged 4 commits into from
May 30, 2017

Conversation

adamryczkowski
Copy link
Contributor

This item was missing after @chovyy 's fix. I named the parameter the same as in the called function, opt_archive which might not be the best name, because most parameters passed by pp_ser.py are optional.

(This is my first ever public PR in Fortran ;-) ).

@chovyy
Copy link
Contributor

chovyy commented May 9, 2017

Sorry, I have never worked with the preprocessor, so I had no idea about that.

@@ -75,17 +76,17 @@ SUBROUTINE ppser_initialize(directory, prefix, mode, prefix_ref, mpi_rank, rprec
IF ( .NOT. ppser_initialized ) THEN
IF ( PRESENT(mpi_rank) ) THEN
WRITE(suffix, '(A5,I0)') "_rank", mpi_rank
CALL fs_create_serializer(directory, TRIM(prefix)//TRIM(suffix), 'w', ppser_serializer)
CALL fs_create_serializer(directory, TRIM(prefix)//TRIM(suffix), 'w', ppser_serializer, opt_archive)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should use a conditional with PRESENT otherwise execution will failed when opt_archive is not passed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, i think you are wrong. When an non-present optional argument is passed to another routine which also declares this argument as optional, the argument will also just be not present in the called routine. So, I'm pretty sure that you don't need an IF (PRESENT(opt_archive)) here, but haven't double-checked it.

ELSE
CALL fs_create_serializer(directory, TRIM(prefix), 'w', ppser_serializer)
CALL fs_create_serializer(directory, TRIM(prefix), 'w', ppser_serializer, opt_archive)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as previous comment

END IF
CALL fs_create_savepoint('', ppser_savepoint)
IF ( PRESENT(mode) ) ppser_mode = mode
IF ( PRESENT(prefix_ref) ) THEN
IF ( PRESENT(mpi_rank) ) THEN
CALL fs_create_serializer(directory, TRIM(prefix_ref)//TRIM(suffix), 'r', ppser_serializer_ref)
CALL fs_create_serializer(directory, TRIM(prefix_ref)//TRIM(suffix), 'r', ppser_serializer_ref, opt_archive)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as previous comment

ELSE
CALL fs_create_serializer(directory, TRIM(prefix_ref), 'r', ppser_serializer_ref)
CALL fs_create_serializer(directory, TRIM(prefix_ref), 'r', ppser_serializer_ref, opt_archive)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as previous comment

@adamryczkowski
Copy link
Contributor Author

Thank you for your detailed review. I hope my update fixes the problem.

@adamryczkowski
Copy link
Contributor Author

Hmm... It compiles on my machine...

@adamryczkowski
Copy link
Contributor Author

OK. I've got the error. Still don't know how on earth did it compiled fine on my machine...

@pspoerri
Copy link
Contributor

pspoerri commented May 9, 2017

Can you also rename opt_archive to archive? Either the argument is present or not.

Copy link
Contributor

@chovyy chovyy left a comment

Choose a reason for hiding this comment

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

Sorry, wasn't able to answer earlier. I think the inserted IFs were not necessary. See my comment on @clementval's review.

@@ -75,17 +76,17 @@ SUBROUTINE ppser_initialize(directory, prefix, mode, prefix_ref, mpi_rank, rprec
IF ( .NOT. ppser_initialized ) THEN
IF ( PRESENT(mpi_rank) ) THEN
WRITE(suffix, '(A5,I0)') "_rank", mpi_rank
CALL fs_create_serializer(directory, TRIM(prefix)//TRIM(suffix), 'w', ppser_serializer)
CALL fs_create_serializer(directory, TRIM(prefix)//TRIM(suffix), 'w', ppser_serializer, opt_archive)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, i think you are wrong. When an non-present optional argument is passed to another routine which also declares this argument as optional, the argument will also just be not present in the called routine. So, I'm pretty sure that you don't need an IF (PRESENT(opt_archive)) here, but haven't double-checked it.

@thfabian
Copy link
Contributor

Whats the status here? Can we merge it?

@havogt
Copy link
Collaborator

havogt commented May 30, 2017

All happy @clementval @pspoerri ?

@clementval
Copy link
Collaborator

@havogt Ok for me.
@chovyy You are right, there are not 100% necessary but it is just clearer like this.

@havogt havogt merged commit 9c80441 into GridTools:master May 30, 2017
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

6 participants