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

Partly fixes #725 #728

Closed
wants to merge 1 commit into from
Closed

Conversation

schlunma
Copy link
Contributor

This PR fixes a part of #725.

@axel-lauer Can you test this please? I do no have an example right now. Thanks! 😄

@schlunma schlunma added the bug label Nov 23, 2018
@schlunma schlunma self-assigned this Nov 23, 2018
Copy link
Contributor

@axel-lauer axel-lauer left a comment

Choose a reason for hiding this comment

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

This change looks fine to me.

@bouweandela
Copy link
Member

This doesn't look good to me. If you're going to use the derive function, that should be implemented for the variable that is requested. Issue #725 is not related to derive functionality, except that it uses the same custom CMOR tables. Therefore, a solution should be implemented separately from derive functionality.

@schlunma
Copy link
Contributor Author

I know that this doesn't solve #725.

All this PR does is to restore the old state before #667, where it was possible to read custom CMOR tables from non-derived variables by setting derive: True in the recipe. It's not a new feature, just a fix to restore the old behavior...

@bouweandela
Copy link
Member

And it makes a mess of the code by mixing functionality. Maybe you can try to implement the what I described here instead?

@schlunma
Copy link
Contributor Author

Yes, I totally agree that this is no final solution. I just wanted to re-add the old workaround which was nice to use until a proper solution is implemented for this problem.

@schlunma schlunma closed this Nov 27, 2018
@schlunma schlunma deleted the version2_fix_derivation branch November 28, 2018 11:21
mattiarighi added a commit that referenced this pull request Jul 22, 2020
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.

3 participants