Added ConfSoilMoisture.py and some edits in Variable.py and ilamblib.py #44
Conversation
There was a problem hiding this comment.
I have taken a look at this. It is a tremendous amount of work and very cleanly implemented. I am a little uncomfortable that so much of it changes things that can affect how ILAMB currently runs. As maybe you are realizing now, as you add models, you find lots of issues with the differences in how models represent things. Much of my code was the way it was to handle weird differences across models. So seeing things that change parts that assume a noleap calendar, makes me nervous because it took a huge amount of work to make the codebase work for all the models.
In the future, if you plan to make changes to parts of the code that are deep (like Variable), let's talk about it so we can make sure that our API stays consistent and that we don't break other areas.
I am inclined to proceed in the following way:
- take a look at my comments in this review and please answer or change as appropriate
- let's remove the YW's and non-meaningful comments (like where code is commented out)
- after this step I will run through my tests and make sure we aren't failing
- then I will run a full analysis (like the CMIP5v6) we have, and make sure everything works as it did before
Presuming all this passes ok, then we will merge this and move on. I have been developing a new version of ILAMB for some time based on xarray. So eventually I will grab this and port it.
| @@ -248,6 +249,68 @@ def MatchRelationshipConfrontation(C): | |||
| found = True | |||
| return C | |||
|
|
|||
| def MatchSensitivityConfrontation(C): | |||
| # YW | |||
There was a problem hiding this comment.
Let's remove the #YW comments here. Your name will be associated with this PR and thus we will always know who to blame :D
| found = True | ||
| return C | ||
|
|
||
| """ |
There was a problem hiding this comment.
Let's remove these lines, thanks for noticing that the function isn't needed. It seems like I got rid of it at some point.
| @@ -43,10 +44,12 @@ def __init__(self, name): | |||
| self.plot_unit = None | |||
| self.space_mean = True | |||
| self.relationships = None | |||
| self.sensitivities = None # YW | |||
| self.keywords = keywords | ||
| self.extents = np.asarray([[-90.,+90.],[-180.,+180.]]) | ||
| self.study_limits = [] | ||
|
|
||
| # Make sure the source data exists | ||
|
|
||
| if not os.path.isfile(self.source): | ||
| try: |
There was a problem hiding this comment.
Is there a reason that what I had wasn't working? I think the isfile is more portable across other systems?
There was a problem hiding this comment.
Your current version should be fine. I did not modify this line. The os.stat() line is a carryover from ILAMB 2.5, which I based my changes on.
| y0 = tdata[0].year | ||
| yf = tdata[1].year | ||
|
|
||
| t = dataset.variables[t.bounds][...] |
There was a problem hiding this comment.
Was what I had in here not working for some data? The issue is that your math here assumes that the calendar is noleap and the datum is 1850-01-01 and that isn't always the case at all. In the future version of ILAMB this will matter less as we will use xarray as the guts.
There was a problem hiding this comment.
This is also a carryover from ILAMB 2.5.
| cmap = 'plasma' if 'plasma' in plt.cm.cmap_d else 'summer') | ||
| norm = LogNorm(), | ||
| cmap = 'plasma' if 'plasma' in plt.cm.cmap_d else 'summer', | ||
| vmin = 1e-4, vmax = 1e-1) |
There was a problem hiding this comment.
Was there some problem with the way I had this? Sometimes things like this are this way to avoid warnings in the output.
There was a problem hiding this comment.
This is also just a carryover from ILAMB 2.5.
| @@ -128,8 +266,11 @@ def __init__(self,**keywords): | |||
| assert variable_name is not None | |||
| t0 = keywords.get("t0",None) | |||
| tf = keywords.get("tf",None) | |||
| z0 = keywords.get("z0",None) | |||
There was a problem hiding this comment.
I had the time range in the Variable constructor because time is the leading dimension in netcdf files and only reading in what you need is a huge savings in memory I/O. If these depth bounds are in the constructor, why not also lat and lon?
There was a problem hiding this comment.
I see. Should I remove the lines 269-270 z0 & zf statements and the z0 & zf arguments into il.FromNetCDF4, and test if things work?
| # Fix potential problems with rolling the axes of the lon_bnds | ||
| self.lat_bnds = self.lat_bnds.clip(- 90,+ 90) | ||
| self.lon_bnds = self.lon_bnds.clip(-180,+180) | ||
|
|
||
| # Make sure that the value lies within the bounds | ||
| assert np.all((self.lat>=self.lat_bnds[:,0])*(self.lat<=self.lat_bnds[:,1])) | ||
| assert np.all((self.lon>=self.lon_bnds[:,0])*(self.lon<=self.lon_bnds[:,1])) | ||
| ##DEBUG |
There was a problem hiding this comment.
let's delete these comments
No description provided.