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

Fix broken CMORizer log message if no Tier directory exists #2207

Merged
merged 4 commits into from
Jul 9, 2021
Merged

Fix broken CMORizer log message if no Tier directory exists #2207

merged 4 commits into from
Jul 9, 2021

Conversation

jmrgonza
Copy link
Contributor

@jmrgonza jmrgonza commented Jun 25, 2021

Python was complaining about 'tier' variable being used before it was defined. I think there is an indentation problem in the 'else' block starting at line 240.

Description

  • Closes #issue_number
  • Link to documentation:

Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the πŸ›  Technical or πŸ§ͺ Scientific review.


To help with the number of pull requests:

Python was complaining about 'tier' variable being used before it was defined. I think there is an indentation problem in the 'else' block starting at line 240.
@bouweandela
Copy link
Member

This is a for else statement and it's intended like that, the warning should only be printed when no data is found in any tier. Maybe you're right and there is some problem with the code, but changing the indentation is not the solution..

@bouweandela
Copy link
Member

Thanks for taking the time to contribute though!

@valeriupredoi
Copy link
Contributor

cheers for contributing! @bouweandela is correct and I am fairly sure you will have a warning saying "Could not find any Tier1,2... dir in raw data..." meaning you should have your data dir ppopulated with Tier2 or Tier3 etc subdirs. In hindsight we should probably break the run right there, if there are no TierX subdirs πŸ‘

@jmrgonza
Copy link
Contributor Author

Hi, thanks for your response.
In fact, the program was breaking for me. To be honest, I didn't know about the 'for ... else' statement, and I think that I now have a better understanding of the logic of the program.
I have looked into the documentation and I think that there is a problem with the 'tier' variable not being defined in the scope of the else block. Also, since the else block of the 'for ... else' statement is executed after the loop executes normally, I think that there should not be a reference to any specific 'tier' in the warning message, meaning that at this point there was no data found for any tier that was tried in the inner for loop.
I will try to propose another fix, if I figure how to amend this pull request.

Modified warning message in for...else statement using a variable out of scope.
Restored original indentation of for...else statement.
Undo last commit
@jmrgonza
Copy link
Contributor Author

Just to give some more detail, I wrote this little function:

def loop(n):
  for i in range(n):
    if i>5:
      break
  else:
    print(i)

Now calling it with some values:

In [3]: loop(10)

In [4]: loop(3)
2

In [5]: loop(0)
---------------------------------------------------------------------------
UnboundLocalError                         Traceback (most recent call last)
<ipython-input-5-60207faaddce> in <module>
----> 1 loop(0)

<ipython-input-2-29a67149e0cc> in loop(n)
      4             break
      5     else:
----> 6         print(i)

UnboundLocalError: local variable 'i' referenced before assignment

Which means that the problem that I found was probably due to not having any tier for the loop to run.

Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Thanks!

@bouweandela bouweandela changed the title Update cmorize_obs.py Fix broken CMORizer log message if no Tier directory exists Jul 9, 2021
@bouweandela
Copy link
Member

@jmrgonza Did you want to add yourself to the list of authors of the ESMValTool? Or are you already on there?

@jmrgonza
Copy link
Contributor Author

jmrgonza commented Jul 9, 2021

Thanks @bouweandela, but I do not really think that making a minor modification to two lines of code should make me an author. I will consider that if I have a chance to make a deeper contribution to the project in the future.

@bouweandela
Copy link
Member

I look forward to that!

@bouweandela bouweandela merged commit 659217a into ESMValGroup:main Jul 9, 2021
@jmrgonza jmrgonza deleted the patch-1 branch July 9, 2021 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants