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

loops over calendar types are unneeded #653

Open
kdraeder opened this issue Mar 11, 2024 · 2 comments · May be fixed by #767
Open

loops over calendar types are unneeded #653

kdraeder opened this issue Mar 11, 2024 · 2 comments · May be fixed by #767
Labels
back burner very low priority. Future work? Good First Issue Good for newcomers pmp possible Marlee project psi possible student issue Refactor Working but needs cleanup

Comments

@kdraeder
Copy link
Contributor

Describe the bug

The time_manager subroutines set_calendar_type_string and get_calendar_string
give the right answers, but the loops over the calendar types (0,...,max_type) are unneeded
and can be confusing.

  1. List the steps someone needs to take to reproduce the bug.
    To see the bug, put print statements in the loops that show the loop index and the input and output variables.
    Build and run a program that handles calendars; filter, model_mod_check, ...
  2. What was the expected outcome?
    When a loop finds the answer it needs, it should exit. Or it should do a different test during each iteration.
  3. What actually happened?
    In set_calendar_type_string the loop exits after the 0th iteration if it matches a calendar,
    or it tries to match the same cstring 6 more times before exiting.
    In get_calendar_string there's a similar loop over calendar types, where the outcome is known after the 0th iteration,
    but it goes on to do the same test 6 more times.

Error Message

See your messages from the suggestion above here.

Which model(s) are you working with?

model_mod_check

Screenshots

  get_calendar_string  : calendar_type =            3
  set_calendar_string  : in types loop i, mystring =            0 GREGORIAN    [the correct string is found on the 0th)
  set_calendar_string  : in types loop i, mystring =            1 GREGORIAN    [ 6 more iterations find the same]
  set_calendar_string  : in types loop i, mystring =            2 GREGORIAN
  set_calendar_string  : in types loop i, mystring =            3 GREGORIAN
  set_calendar_string  : in types loop i, mystring =            4 GREGORIAN
  set_calendar_string  : in types loop i, mystring =            5 GREGORIAN
  set_calendar_string  : in types loop i, mystring =            6 GREGORIAN

Version of DART

v9.12.refactored-1264-g38f4139ba

Have you modified the DART code?

Only to print messages. I committed those to branch calendar_loops.

Build information

Please describe:

  1. The machine you are running on
    Mac laptop, but this happens on any machine.
  2. The compiler you are using (e.g. gnu, intel).
    gfortran, but this will happen with any compiler (unless it recognizes a spurious loop)

The intention may have been to have an array of calendar_types(0:max_type),
then use variable calendar_type to access an element of it without the explicit if-blocks,
but somehow that was not implemented, or was partially removed.
Removal of the loops would be a simple fix. Implementation of the calendar_types array
would be more involved, but would probably result in smaller or more elegent code.
That might be an opportunity to make NO_CALENDAR = 0 and let actual calendars have 1,...,max_type

@kdraeder kdraeder added Good First Issue Good for newcomers Refactor Working but needs cleanup back burner very low priority. Future work? labels Mar 11, 2024
@hkershaw-brown
Copy link
Member

just a heads up for anyone looking at the branch calendar_loops

calendar_loops...main calendar_loops is branched before the quantile_methods release.

@kdraeder
Copy link
Contributor Author

I did not look through time_manager_mod for more examples of these extra loops,
or suboptimal use of calendar types.

@hkershaw-brown hkershaw-brown added the psi possible student issue label Apr 5, 2024
@hkershaw-brown hkershaw-brown added the pmp possible Marlee project label Oct 23, 2024
@mjs2369 mjs2369 linked a pull request Nov 5, 2024 that will close this issue
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back burner very low priority. Future work? Good First Issue Good for newcomers pmp possible Marlee project psi possible student issue Refactor Working but needs cleanup
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants