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

Renaming CTSM modules to avoid namespace collisions [WIP] #1143

Closed
wants to merge 2 commits into from

Conversation

negin513
Copy link
Contributor

@negin513 negin513 commented Sep 9, 2020

Description of changes

For handling possible namespace collisions when using CTSM with other models, @billsacks suggested to add ctsm_ prefix to all modules. This is especially important for some modules that were named generically such as fileutils or abortutils.

This PR includes other improvements for better naming the ctsm files. This PR also includes renaminging the files containing these modules.
The following repository includes all the modules and files old and new names.
https://github.com/negin513/ctsm_filenames/blob/master/ctsm_filenames.csv

Specific notes

Contributors other than yourself, if any:

CTSM Issues Fixed (include github issue #): #869

Are answers expected to change (and if so in what way)? No

Any User Interface Changes (namelist or namelist defaults changes)? No

Testing performed, if any:
aux_clm is tested on Cheyenne comparing with ctsm1.0.dev108

@negin513 negin513 changed the title Renaming CTSM modules to avoid namespace collisions (WIP) Renaming CTSM modules to avoid namespace collisions [WIP] Sep 9, 2020
@negin513
Copy link
Contributor Author

negin513 commented Sep 9, 2020

  • The are some use statements, etc, which were previously aligned but due to the probably longer names, they are not aligned anymore. For example,

! ctsm uses
use ncdio_pio , only : pio_subsystem
use ctsm_Domain , only : ldomain
use ctsm_TimeManager , only : get_calendar

@billsacks
Copy link
Member

I wouldn't worry about the misaligned use statements: I think it's too much of a pain to try to keep these aligned, for limited benefit other than aesthetics.

@ekluzek
Copy link
Contributor

ekluzek commented Sep 10, 2020

I wouldn't worry about the misaligned use statements: I think it's too much of a pain to try to keep these aligned, for limited benefit other than aesthetics.

I have put a lot of effort into making these things align. And I agree I'm not sure how important it is. It does look better to the eye. And sometimes this type of alignment does help in identifying problems in code that should look the same. For use statements I'm not sure it matters much though, since there aren't as many things to make sure are the same.

But, since you are doing this with a script I agree that you shouldn't spend a lot of effort getting these things to align. If we work on the code later by hand -- we could realign by hand later.

I do think it would be good to get us to agree as to whether we should align use statements in general. Then I could not worry about this at all in the future. Right now I do try to get them to align since that's been the standard in the code. So I'll add this as a discussion point for CTSM software.

@billsacks billsacks added this to In progress - master in Upcoming tags Sep 24, 2020
@billsacks
Copy link
Member

I have taken a quick look at this. I don't think I can really give it a careful review (my eyes would glaze over). As long as the tests are passing, I'd be fairly confident that there aren't any real issues. As for reviewing the naming, I figure that, if we find a few names we're unhappy with once we start working with this over the next few months, we can make a few renames at that point.

I do want to discuss a bigger point, though (probably at the ctsm-software meeting this Thursday, based on looking at directory listings, etc.): Now that I'm coming back to this 9 months after we started talking about it, I'm questioning whether it makes sense to include the ctsm_ prefix on file names as well as module names. It is needed on modules to avoid namespace collisions, but it isn't required on file names. I'm on the fence about this myself, but just want to make sure we're all happy with this before moving ahead.

Here are pros & cons I see about having the ctsm_ prefix on file names:

  • Pros:
    • Keeps file name same as module name (I view this as minor: it is
      pretty easy to learn that the translation from module name to
      file name involves dropping the 'ctsm_' prefix)
    • In directory listings, separates the main files from the handful
      of other files (CMakeLists.txt, tests directory)
  • Cons:
    • Need to ignore this prefix when browsing through file names
    • A bit more typing every time you open a new file
    • An additional pain point if we were to ever rename CTSM

For what it's worth, I looked at pFUnit as an example of what others do, because I feel Tom Clune is very thoughtful about things like this. pFUnit uses prefixes of pf_ for the module names, but excludes this prefix on the file names (e.g., see the files in https://github.com/Goddard-Fortran-Ecosystem/pFUnit/tree/main/src/funit/core).

Again, I'm on the fence about this, but I feel we should have one more quick discussion of this before finalizing it.

@billsacks
Copy link
Member

billsacks commented Oct 15, 2020

From discussion at today's ctsm-software meeting: General feeling is to keep the prefix. One nice thing about it is that it distinguishes between files in CTSM vs. FATES.

@ekluzek ekluzek marked this pull request as draft September 2, 2021 19:37
@ekluzek ekluzek added the tag: next this issue should get some attention in the next week or two label Apr 14, 2022
@ekluzek
Copy link
Contributor

ekluzek commented Apr 14, 2022

We did touch on this in our Tuesday standup meeting. This is currently not urgent, but is still something that we should do. This will make it easier to adopt any atmosphere model coupled to CTSM via LILAC. So there is no pressing need for it, but still something useful to do. And doing this change will help with code readability and adhering to our coding standards.

@billsacks billsacks removed the tag: next this issue should get some attention in the next week or two label May 5, 2022
@billsacks
Copy link
Member

I know we have discussed this twice already, but coming back to this with a fresh set of eyes once again, I am going to cast a somewhat stronger vote this time for removing the "ctsm_" prefix from file names (so module ctsm_MyModule would appear in MyModule.F90 rather than ctsm_MyModule.F90). (See #1143 (comment) for more discussion on this.) I still don't feel super strongly about this, so if most others still prefer keeping the "ctsm_" prefix on file names I'm okay with that, but I feel like this makes it harder to skim through a list of file names and pick out the one you want, since the leading prefix is the same on all of them.

@ekluzek
Copy link
Contributor

ekluzek commented Oct 17, 2022

@negin513 this is another one that we should take on. Is everything needed inside this PR? I'm especially wondering about the script that changes the files?

@ekluzek
Copy link
Contributor

ekluzek commented Oct 17, 2022

Ahh, OK reading the top introduction I see the location of the repo with the script.

@rgknox
Copy link
Contributor

rgknox commented Oct 17, 2022

I took a look at our fates folder to see if we are doing the same thing. We are kinda imposing the Fates prefix. About 50%-ish of the time we prefix with Fates, maybe 25% we prefix with ED (older files, predecessor to FATES), fire files start with SF (for spitfire), allocation files start with PRT and two files use no prefixing (bad).

@billsacks billsacks removed this from In progress - master in Upcoming tags Jan 5, 2023
@ekluzek ekluzek added the tag: next this issue should get some attention in the next week or two label Jan 9, 2023
@ekluzek
Copy link
Contributor

ekluzek commented Jan 9, 2023

We think this is something that we will have to get back to at a later date when we know who can work on this.

@billsacks
Copy link
Member

From discussion at today's ctsm-software meeting: we do still want to do this, but will probably have to redo it. @negin513 's python script will still be useful to help with this, but we're going to close this PR for now and plan to redo this at some point.

@billsacks billsacks closed this Mar 2, 2023
@billsacks billsacks removed the tag: next this issue should get some attention in the next week or two label Mar 2, 2023
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

4 participants