Conversation
mjprilliman
left a comment
There was a problem hiding this comment.
Looks good, thanks for figuring this out! A few side effects of this:
With this change, 'Heat map' now has an extra Jan label at the end of the monthly timeseries:
I managed to crash SAM when clicking and zooming around in the white space beyond the last data point, but I can't replicate it. This behavior is the same as before, so maybe it was just a fluke. Is that white space beyond the last data point required?
On the Solar Resource 'View data' Dview pop-up, on the Daily tab, scrolling to the end and clicking the right arrow at the end of the annual timeseries seems to cut it off at Dec 30, rather than 31st:
There is also strange behavior on the 'Monthly' tab, but this happens in the release as well. Do we need three tabs for this data viewer, or would it be possible to disable the zoom for daily and monthly?
|
|
||
| // to skip leap years (SAM issue 289) - scale to first year | ||
| int m_min_offset = m_min / 8760; | ||
| double m_min_scaled = (double)((int)m_min % 8760); // m_min and m_max always based on 8760 (365 days with no leap years) |
There was a problem hiding this comment.
Likely a different issue, but there's hopes to actually handle leap days in the calculations this year. This would likely require an additional flag to this function to scale to 8784 if a leap year is detected, and base each year on a leap year instead (8784 * timesteps_per_hour * nyears).
There was a problem hiding this comment.
Likely a different issue, but there's hopes to actually handle leap days in the calculations this year. This would likely require an additional flag to this function to scale to 8784 if a leap year is detected, and base each year on a leap year instead (8784 * timesteps_per_hour * nyears).
Good catch and way to use existing code. If we do leap years, we can probably assign a base year and then use the full wxDateTime class to handle as it was designed.
I think we need zoom on the Daily and Monthly tabs for lifetime datasets so you can zoom in to a single year at a time. Daily and monthly averages are useful for some analysis. |
@mjprilliman, thanks for the detailed review. With the latest commit, I think all issues have been addressed with the following comments: Heat map no longer has extra Jan at end of lifetime data The extra white space beyond the last data point is required for last tick mark of axis on a day boundary. With the latest update, all the weather files I tested had Dec 31 The monthly tab is acting a little weird (Jan and Dec half lengths) because the data is in the middle of each month Another issue we can create from here is when switching from lifetime to hourly data in timeseries or heat maps, if the lifetime data is scrolled beyond the first year, then the hourly data is blank and can be recaptured using zoom to fit: switch to hourly load data click zoom fit |
mjprilliman
left a comment
There was a problem hiding this comment.
I think everything works as intended, and looks good in the display. Thanks for fixing this!
Please test with lifetime, single year, hourly and sub-hourly files.
Please test for Feb 29 in out years as described in NatLabRockies/SAM#289
Please test transitions between years (Dec to Jan) at various zoom levels.
Test file - detailed PV residential here
SAM_289.zip