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

PrePARE issues #332

Closed
taylor13 opened this issue Mar 29, 2018 · 11 comments
Closed

PrePARE issues #332

taylor13 opened this issue Mar 29, 2018 · 11 comments
Assignees
Labels
Projects

Comments

@taylor13
Copy link
Contributor

@taylor13 taylor13 commented Mar 29, 2018

I reviewed the python PrePARE code and noticed the following potential problems:

  1. I think the missing_value and _FillValue attribures are checked against the CMIP6 specified value. These are both float32 variables, and there is an if test for equivalence as follows:
 if 1 - (table_value / file_value) < 0.00001:

which is preceded by a test to trap the case of file_value=0.

This test is not rigid enough because, for example, a file_value = table_value/100. will satisfy the test even though we want the table_value and file_value to be the same. The expression on the left side of the if test comparison should be inside an absolute value function.

  1. In testing whether time-label portion of the filename is consistent with time coordinates stored in the file, the code expects the bounds to be stored in a variable named "time_bnds" or "climatology_bnds". Currently the output specifications don't actually require these names; they require that the bounds be stored in a variable named in the "bounds" attribute or the "climatology" attribute. I wonder if we can modify PrePARE to extract the name of the bounds variable instead of having it hard wired. Is it too late to require these as the bounds names?
@glevava

This comment has been minimized.

Copy link

@glevava glevava commented Mar 30, 2018

Hi Karl, regarding point 2 of your issue, I already add the following fix on my PrePARE instance (be part of my next PR):

        # Get first and last time bounds
        try:
            if 'bounds' in infile.variables['time'].__dict__.keys():
                bndsvar = infile.variables['time'].__dict__['bounds']
                startimebnds = infile.variables[bndsvar][0][0]
                endtimebnds = infile.variables[bndsvar][-1][1]
            else:
                startimebnds = infile.variables['time_bnds'][0][0]
                endtimebnds = infile.variables['time_bnds'][-1][1]
        except BaseException:
            startimebnds = 0
            endtimebnds = 0
@glevava

This comment has been minimized.

Copy link

@glevava glevava commented Mar 30, 2018

For the point 1, do you mean it should be:
if abs(1 - (table_value / file_value)) < 0.00001:

If yes, I will include this change in my PR.

@taylor13

This comment has been minimized.

Copy link
Contributor Author

@taylor13 taylor13 commented Mar 30, 2018

Yes, that was my suggestion. The intent is to make sure file_value is the same (or almost the same) as table_value. Note that the code shown above is preceded by a test for a special case. We could also eliminate the test for the case when file_value is 0. by writing it:

if abs(table_value - file_value) < 0.00001*table_value 

This would require file_value to be equal to table_value when table_value=0., and when not, the two would be required to very nearly be the same (i.e., to within 0.001%). The approximate condition will allow for truncation errors. These truncation errors should be avoidable, so perhaps we should raise a warning when exact equivalence is not found. What do you think?

@taylor13 taylor13 closed this Mar 30, 2018
@taylor13 taylor13 reopened this Mar 30, 2018
@taylor13

This comment has been minimized.

Copy link
Contributor Author

@taylor13 taylor13 commented Mar 30, 2018

closed by mistake, but the original fix has been submitted as a pull request by @glevava

@dnadeau4

This comment has been minimized.

Copy link
Collaborator

@dnadeau4 dnadeau4 commented Apr 11, 2018

This has been merged with @glevava solution.
b11e29e

@dnadeau4 dnadeau4 closed this Apr 11, 2018
@taylor13

This comment has been minimized.

Copy link
Contributor Author

@taylor13 taylor13 commented Apr 11, 2018

Might use my equation that eliminates the if test for 0.

@taylor13 taylor13 reopened this Apr 11, 2018
@dnadeau4 dnadeau4 self-assigned this Apr 25, 2018
@dnadeau4 dnadeau4 added the PrePARE label Apr 25, 2018
@taylor13

This comment has been minimized.

Copy link
Contributor Author

@taylor13 taylor13 commented Apr 30, 2018

To be more general (e.g., for cases where the table_value<0), we need another abs function in the test:

if abs(table_value - file_value) < 0.00001*abs(table_value) 
dnadeau4 added a commit that referenced this issue May 2, 2018
@doutriaux1

This comment has been minimized.

Copy link
Contributor

@doutriaux1 doutriaux1 commented Jun 28, 2018

@dnadeau4 is c38e4ad part of any branch? Is that in the stuff you're going to merge from the prepare branch? Should I take over that branch and do the merge myself?

@dnadeau4

This comment has been minimized.

Copy link
Collaborator

@dnadeau4 dnadeau4 commented Jul 3, 2018

@doutriaux1 this is done, not sure why @taylor13 reopened it. The climatology was merged but guillaume changed a couple of line that need to be merged again and all tests need to be run.

@taylor13

This comment has been minimized.

Copy link
Contributor Author

@taylor13 taylor13 commented Jul 7, 2018

I haven't rechecked, but last time I looked the correct if-test specified in #332 (comment) had not been implemented. That's why I reopened.

@doutriaux1 doutriaux1 added this to Done in 3.4 Jul 18, 2018
@doutriaux1 doutriaux1 moved this from Done to In progress in 3.4 Jul 18, 2018
@doutriaux1 doutriaux1 moved this from In progress to Needed in 3.4 Jul 18, 2018
@doutriaux1 doutriaux1 moved this from Needed to In progress in 3.4 Jul 18, 2018
@taylor13

This comment has been minimized.

Copy link
Contributor Author

@taylor13 taylor13 commented Aug 7, 2018

In #332 (comment) , the < symbol should be replaced with symbol for less than or equal to. Thanks, @doutriaux1 , for catching the error.

with this change the test will be passed even if both the fill value and table value are 0.0

@taylor13 taylor13 assigned mauzey1 and unassigned dnadeau4 Oct 11, 2018
3.4 automation moved this from In progress to Done Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3.4
  
Done
5 participants
You can’t perform that action at this time.