NOAA-GSL / ExascaleWorkflowSandbox

Other
2 stars 2 forks source link

leadtime utility functions should be renamed and need tests #86

Closed christopherwharrop-noaa closed 4 months ago

christopherwharrop-noaa commented 5 months ago

The leadtime.fcst_to_seconds and leadtime.seconds_to_fcst utility functions should be renamed to better reflect what they do. These functions convert between the string representation of a forecast lead time and the equivalent number of seconds. Therefore better names would be: leadtime.leadtime_to_seconds and leadtime.seconds_to_leadtime.

NOTE: These functions are used extensively in feature/qg and elsewhere. Renaming them will require changing a lot of calls to these functions.

Additionally, these utility functions need a comprehensive set of tests to ensure they work correctly. The tests should cover (at least) the following scenarios:

leadtime.leadtime_to_seconds

leadtime.seconds_to_leadtime

NaureenBharwaniNOAA commented 5 months ago

The leadtime.fcst_to_seconds and leadtime.seconds_to_fcst utility functions should be renamed to better reflect what they do.

I was thinking an initial approach to this would be to perform a grep -le 'leadtime.fcst_to_seconds' action. I thought this would provide me with all the instances of where this occurs and then I could do a simple search and replace across multiple files. However, I haven't had much luck getting this command to work and usually just sits there and I wait for it to return. It's possible the list is long, but even searching the repo on GitHub I get no results back.

I was able to perform a simple search in the command line across the chiltepin folder and echo the status returned.

admin@slurmfrontend:~/chiltepin$ grep -le 'John' setup.cfg 
admin@slurmfrontend:~/chiltepin$ echo $? grep -le 'John' setup.cfg 
0 grep -le John setup.cfg

Wondering if this is the best way to go about the initial name change?

christopherwharrop-noaa commented 5 months ago

This is one of the times when using a IDE like VS Code, Intellij, or Eclipse comes in handy. I know that Intellij has a dropdown menu that you can use to "rename" methods and variables, and it will automatically change all references to it in the entire codebase. I am looking into figuring out how to get VS Code setup in remote mode so we can use it to work with code on Hera or Hercules while running the IDE application on our laptop.

But... without that, the best way is to just grep the code with something like grep -r 'leadtime.fcst_to_seconds' at the root of the repo.

NaureenBharwaniNOAA commented 5 months ago

Interesting, the grep command worked when working in a container, but I now switched over to Hera after the rebase since I thought it would be easier in the long run and the command is also stuck in limbo right now at the root of the repository. In the container it was able to return a result easily.

it eventually worked and using this article I was able to perform the command

(base) hfe05:/scratch1/BMC/zrtrr/Naureen.Bharwani/SENA/ExascaleWorkflowSandbox $ grep -rl leadtime.fcst_to_seconds . | xargs sed -i 's/leadtime.fcst_to_seconds/leadtime.leadtime_to_seconds/g'

Then verify the naming switch output:

(base) hfe05:/scratch1/BMC/zrtrr/Naureen.Bharwani/SENA/ExascaleWorkflowSandbox $ grep -r 'leadtime.fcst_to_seconds'
(base) hfe05:/scratch1/BMC/zrtrr/Naureen.Bharwani/SENA/ExascaleWorkflowSandbox $ grep -r 'leadtime.leadtime_to_seconds'
src/chiltepin/jedi/qg/workflow.py:    0, leadtime.leadtime_to_seconds(experiment.config["experiment"]["length"])
src/chiltepin/jedi/qg/workflow.py:        0, leadtime.leadtime_to_seconds(experiment.config["experiment"]["frequency"])
src/chiltepin/jedi/qg/osse.py:        exp_length = leadtime.leadtime_to_seconds(self.config["experiment"]["length"])
src/chiltepin/jedi/qg/osse.py:        exp_freq = leadtime.leadtime_to_seconds(self.config["experiment"]["frequency"])
src/chiltepin/jedi/qg/osse.py:            0, leadtime.leadtime_to_seconds(self.config["assimilation"]["window"]["begin"])
src/chiltepin/jedi/qg/osse.py:            # prev_cycle = t - timedelta(0, leadtime.leadtime_to_seconds(exp_freq))
src/chiltepin/jedi/qg/osse.py:            0, leadtime.leadtime_to_seconds(self.config["assimilation"]["window"]["begin"])
src/chiltepin/jedi/qg/osse.py:            leadtime.leadtime_to_seconds(self.config["assimilation"]["window"]["begin"])
src/chiltepin/jedi/qg/osse.py:            + leadtime.leadtime_to_seconds(self.config["experiment"]["frequency"])
src/chiltepin/jedi/qg/osse.py:            + (leadtime.leadtime_to_seconds(window_length) // 2)
src/chiltepin/jedi/qg/osse.py:                0, leadtime.leadtime_to_seconds(self.config["experiment"]["frequency"])
src/chiltepin/jedi/qg/osse.py:        prev_cycle = t - timedelta(0, leadtime.leadtime_to_seconds(exp_freq))
(base) hfe05:/scratch1/BMC/zrtrr/Naureen.Bharwani/SENA/ExascaleWorkflowSandbox $