DM-43319: Fix warnings #103
Conversation
kkeijuro
force-pushed
the
tickets/DM-43319
branch
from
79e19f2
to
928f301
Compare
config/quickLookIsr.py
Outdated
| @@ -3,3 +3,4 @@ | |||
| config.doSaturationInterpolation = True | |||
| config.overscan.fitType = "MEDIAN_PER_ROW" | |||
| config.overscan.doParallelOverscan = True | |||
| # config.brighterFatterMaxIter = 2 # Uncomment this to remove test warning | |||
There was a problem hiding this comment.
I think this should go back in - that's an acceptable warning to have.
| elif hasattr(dataId, "items"): # dafButler.dimensions.DataCoordinate | ||
| return {str(k): v for k, v in dataId.items()} # str() required due to full names | ||
| elif hasattr(dataId, "dataId"): # dafButler.DimensionRecord | ||
| return {str(k): v for k, v in dataId.dataId.items()} | ||
| elif hasattr(dataId, "dataId"): # dafButler.dimensions.DimensionRecord |
There was a problem hiding this comment.
Why did you add the .dimensions here? It's exported to the module level, so it wasn't wrong before.
| plt.figure(figsize=(10, 6)) | ||
| plt.figure(figsize=(16, 12)) |
There was a problem hiding this comment.
What is the purpose of changing the figure size here and below? I wonder if there's a better fix, as this will probably have other consequences. I suspect these defaults were picked quite carefully (and they're also not the same, and you've changed them to be the same).
There was a problem hiding this comment.
it complains about figure size, I put the warning below. I tried several numbers and found those which are the nearest to the originals (10/6) that doesnt create the warning. The only problem is that they dont have same relation. 1.34 vs 1.67
tests/test_nightReport.py::NightReportTestCase::test_plotPerObjectAirMass
/sdf/home/c/cpio/development/summit_utils/python/lsst/summit/utils/nightReport.py:654: UserWarning: Tight layout not applied. The bottom and top margins cannot be made large enough to accommodate all axes decorations.
plt.tight_layout()
There was a problem hiding this comment.
OK, thanks. It's not the most important plot to be honest, so let's leave it like this, and if it looks bad we can fix it up later, thanks.
tests/test_butlerUtils.py
Outdated
| # to avoid getting the next Warning: | ||
| # UserWarning: FigureCanvasAgg is non-interactive, and thus cannot be shown | ||
| # plt.show() | ||
| plt.show = custom_show |
There was a problem hiding this comment.
As with the other review, I wonder if we can just remove the calls to .show() instead.
There was a problem hiding this comment.
So we should remove the show inside the code? The show() is not part from the test but from the function in the library. I guess you mean returning the matplotlib Plot and then the uplayer call the show() from it, (but that is a big change)
There was a problem hiding this comment.
I think that's an acceptable change - I don't think any of this library code should be calling show(), and so I think removing it is fine, and if things upstream break, that fixing them there is the right way to do things.
| @@ -322,7 +322,7 @@ def test_endToEnd(self): | |||
| @vcr.use_cassette() | |||
| def test_noDataBehaviour(self): | |||
| eventMaker = self.tmaEventMaker | |||
| noDataDayObs = 19500101 # do not use 19700101 - there is data for that day! | |||
| noDataDayObs = 19600101 # do not use 19700101 - there is data for that day! | |||
There was a problem hiding this comment.
Why did you change this?
There was a problem hiding this comment.
this is the problem:
UTC began at 1960 January 1.0 (JD 2436934.5) and it is improper
** to call the function with an earlier date. If this is attempted,
** zero is returned together with a warning status.
And warning is shown:
/sdf/group/rubin/sw/conda/envs/lsst-scipipe-8.0.0/lib/python3.11/site-packages/erfa/core.py:154: ErfaWarning: ERFA function "d2dtf" yielded 1 of "dubious year (Note 5)"
warnings.warn('ERFA function "{}" yielded {}'.format(func_name, wmsg),
The date selected is inside UTC (but with no data since it begins on 19700101)
There was a problem hiding this comment.
OK, that sounds good then, thanks!
mfisherlevine
force-pushed
the
tickets/DM-43319
branch
from
f400fa2
to
64f8475
Compare
No description provided.