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

Support time-series layers #187

Merged
merged 20 commits into from
Sep 24, 2019
Merged

Conversation

ojustino
Copy link
Contributor

@ojustino ojustino commented Mar 4, 2019

@astrojonathan Here is an environment where you can test pywwt directly to figure out what's going on. I've added console.log statements to the wwt_json_api.js file from earlier that should print messages to the console to let you know what's being passed to the SDK. The instructions to set it the time series test are as follows.

Install the developer version of pywwt in terminal (if you don't have it already):

git clone https://github.com/WorldWideTelescope/pywwt.git
cd pywwt
pip install -e .

Clone this pull request branch to your machine:

git checkout -b ojustino-layer-time master
git pull https://github.com/ojustino/pywwt-web.git layer-time

It's possible to run pywwt via script, but it might be better to troubleshoot interactively on this one, so go to the pywwt/nbextension/static directory, open IPython on the command line, and then copy/paste as instructed from time_test.py.

A Qt window running the same wwt.html file we worked from last week will pop up once you've copied, pasted, and run the code block with the imports from time_test.py.

@ojustino ojustino force-pushed the layer-time branch 4 times, most recently from aaac92a to 00c845a Compare August 11, 2019 00:17
@ojustino
Copy link
Contributor Author

ojustino commented Aug 11, 2019

The first three of the new commits are focused on making what I hope are small improvements in trait functionality by expanding the docstrings for TableLayer traits, making the error messages in AstropyQuantity and Color more informative, and resolving some JS TypeErrors that arose from the wwt.*ground attributes having a flag they shouldn't have.

@pkgw, the most notable development is that I got time series layers to show up in the Qt version. I was also able to get decay working, so, if the user wants, they can make it so points disappear after a specific amount of time. (I also added time_test.ipynb to verify that things were still working for a Jupyter notebook in Firefox.)

I hear the concern in #175 about parsing dates from strings, but I don't think there's another way to do it that works for the Qt version. My solution verifies that those strings all conform to the ISOT standard before passing them to WWT. ISOT is browser-independent and should hopefully then be friendly with WebKit, Chromium, and Firefox.

As for time zones in Qt, I used datetime to find the user's current time and add the offset between that and the current UTC time to values in the time column before passing the column to WWT. The current drawback of this approach is Daylight Saving Time. If the user's current DST status is not the same as it is for the dates in their time column, there's a one hour offset. (The test script for the Qt version illustrates this discrepancy since the events are set in January.) It's also assumed that users are providing UTC times, but I hope that can be expanded in the future.

Other questions I have are marked in comments beginning with 'ISSUE' in layers.py, but that's the general summary. It would be great if @astrofrog could take a look and see how my implementation fits with how the rest of that file was set up.

Would resolve #175 once merged.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

I agree with the general approach here, and have left some comments below.

I don't fully understand why time zones are relevant, so this could do with some more explaining in the code. I think we should just assume that the times passed to us are in UTC, and WWT should interpret them as such (not as local times). However, I just realized that I had an issue for another project that was related to this, and the problem was that the time strings should have a 'Z' added at the end to indicate UTC, otherwise Javascript gets confused.

However, most users don't know to add the 'Z', and astropy doesn't, so I think this is something we should probably do when we serialize and send the table to WWT. Can you try that and see if the hacks related to time zones are still needed?

pywwt/layers.py Outdated Show resolved Hide resolved
pywwt/layers.py Show resolved Hide resolved
pywwt/layers.py Outdated Show resolved Hide resolved
pywwt/layers.py Outdated Show resolved Hide resolved
pywwt/layers.py Outdated Show resolved Hide resolved
pywwt/layers.py Outdated Show resolved Hide resolved
pywwt/nbextension/static/time_test.ipynb Outdated Show resolved Hide resolved
pywwt/nbextension/static/time_test.py Outdated Show resolved Hide resolved
@ojustino
Copy link
Contributor Author

ojustino commented Sep 14, 2019

OK, I changed wwt.set_current_time in core.py to also pass an ISOT string to WWT. (I also made a corresponding change in wwt_json_api.js .) I also integrated the pytz package to help verify/convert time zones for values provided to that method and in the time column of a table layer. These steps eliminated the issues I was having with DST, and the points from the table in the time_test files now pop up when they should in both Qt and Jupyter (on Firefox).

If this looks close to the implementation we'll use going forward, I can go ahead and update the docs (for both table and image layers) and update the change log.

I wasn't sure how to handle adding a 'Z' to ISOT strings when some of them may already have a 'Z' or a '-05:00' at the end for time zone specification, so I didn't yet do so. (See next message.)

I'm not sure if things are now synchronized because both wwt.set_current_time and the time series layer are in UTC or because they're both in the user's local time. Is there a simple way to test this? (Using toLocaleString() and toUTCString() to verify Date objects created in wwt_json_api.js.)

@ojustino
Copy link
Contributor Author

ojustino commented Sep 16, 2019

I went ahead and added a method to core.py that returns the current time in the viewer -- wwt.get_current_time(). I followed @jsub1's methodology from #206 to ensure it works in Jupyter.

After reinstalling the directory with pip (due to changes to wwt.js), it seems to work as intended.


I also made more edits to TableLayer so it can now read time columns made from a) astropy Time, b) datetime.datetime (with or without time zone information), or c) UTC ISOT strings. It then turns the column entries into ISOT strings with a specified "+0:00" time zone (as suggested by @astrofrog). This removes the discrepancies in time between the Qt and Jupyter versions and ensures we are always working in UTC. I also updated the time_test notebooks in the static/ directory in case you all would like to test the changes yourselves.

All that's left out is ISOT strings with time zones (e.g. "2014-06-30T22:00:34+03:00"). This is because I use astropy's Time to verify them, and Time seems to only accept ISOT strings with no time zone info or with a 'Z' for UTC at the end. Should we try to support them, too?

I also corrected some tests in the validation of time_att that weren't doing what I thought they were.

@pkgw
Copy link
Contributor

pkgw commented Sep 16, 2019

@ojustino OK, the CI is now getting a bit farther. It's now erroring out because it thinks that some of your test files are pytest tests and tries to run them as such, which doesn't work. If you're ready to remove your test files, doing so would help the CI make more progress.

@ojustino
Copy link
Contributor Author

@pkgw OK, I guess they'll still be there in the old commits. Did you get a chance to test out the time series functionality or wwt.get_current_time? If you or @astrofrog could, it would be helpful.

@pkgw
Copy link
Contributor

pkgw commented Sep 18, 2019

OK, I've been exploring this and am seeing some weird stuff that I think is a lot more about how the WWT engine works than the pywwt layer. For reference, here's the outline of a test notebook I'm using:

# ---- new cell ----
from pywwt.jupyter import WWTJupyterWidget
import numpy as np
from astropy import units as u
from astropy.table import Table
from astropy.time import Time

# ---- new cell ----
wwt = WWTJupyterWidget()
wwt.pause_time()
t0 = wwt.get_current_time()
wwt

# ---- new cell ----
n = 30
TIMESPAN = 100
ra_deg = np.linspace(0, 40, n)
dec_deg = np.zeros(n)
unix_times = t0.unix + np.linspace(0, TIMESPAN, n)
iso_times = [Time(ut, format='unix').isot for ut in unix_times]
table = Table({'ra': ra_deg, 'dec': dec_deg, 'time': iso_times})
layer = wwt.layers.add_data_layer(
    table,
    lon_att = 'ra', lat_att = 'dec', lon_unit = u.deg,
    time_att = 'time', time_series = True,
    size_scale = 60
)

# ---- new cell ----
wwt.set_current_time(t0)
wwt.play_time()

I'll see a dot show up, but in clumps instead of uniformly ... does WWT's time system lose precision at the few-second level? I'd be surprised.

Still trying to understand what's going on, but I wanted to write down the example that I'm looking at.

Copy link
Contributor

@pkgw pkgw left a comment

Choose a reason for hiding this comment

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

A couple of miscellaneous comments, but as per my previous comment I'm having trouble getting this feature to work reliably and I'm still trying to track down what's going on — I think it might be more about WWT and not this PR so much, but need to investigate.

pywwt/core.py Outdated Show resolved Hide resolved
pywwt/layers.py Show resolved Hide resolved
pywwt/layers.py Show resolved Hide resolved
pywwt/layers.py Outdated Show resolved Hide resolved
@ojustino
Copy link
Contributor Author

ojustino commented Sep 18, 2019

@pkgw I went over the results of pytest and fixed some that were failing due to changes I made in this PR. There are still 6 of 48 that fail on my machine (a few mismatched images and a few that don't touch on time series layers), though on Azure it's just 1/48 (a mismatched image).

I also updated the change log and moved the entry under 0.6.2 to 0.7.0 since it looks like we're going to skip the former.

EDIT: I didn't see your review until after I wrote this, but I'll follow up tomorrow if I can.

@ojustino
Copy link
Contributor Author

ojustino commented Sep 18, 2019

The same behavior happens for me with your example, but if you try time_test.ipynb from commit b811132, the points appear one-by-one even though most are within a few (<5) seconds of one another. One difference in mine is that it's in solar system mode, but the points don't clump even if I change the frame from 'Earth' to 'Sky'.

@ojustino
Copy link
Contributor Author

Actually, your test now works as intended for me. Did you use pip install -e . after pulling the most recent changes to the PR? I tested on a different machine and that's what fixed things. (I could tell something was wrong because wwt.get_current_time() wasn't changing, and it only works after a fresh install since it involves some changed code in wwt.js.)

@pkgw pkgw changed the title Troubleshooting time series layer behavior Support time-series layers Sep 20, 2019
@pkgw pkgw self-requested a review September 21, 2019 19:11
pkgw and others added 7 commits September 21, 2019 19:41
A CI failure revealed a weakness in the Jupyter validation code: if there's a
problem, the code will crash if you haven't passed it a logger, causing the
true error message to be lost. So, let's provide a logger.
Document that, and other recently-added requirements as well.
@ojustino
Copy link
Contributor Author

ojustino commented Sep 21, 2019

This was an arduous rebase; I have to do it over. The deed is done; feel free to continue testing. It might be easier to just create a new branch and pull what I've pushed here.

@pkgw
Copy link
Contributor

pkgw commented Sep 22, 2019

OK, I was able to track down my problem. It is in fact a precision issue in the WebGL shader implementation in the WWT SDK; the date calculations in SpreadSheetLayer are done relative to a baseDate field that is hardcoded to be January 1, 2010. Date differences that exceed floating-point precision away from that date will expose roundoff errors. So, if I change my example to fix it dates to be near baseDate, everything works OK:

# ---- new cell ----
from pywwt.jupyter import WWTJupyterWidget
import numpy as np
from astropy import units as u
from astropy.table import Table
from astropy.time import Time

t0 = Time('2010-01-01T12:00:00.000', format='isot')

# ---- new cell ----
wwt = WWTJupyterWidget()
wwt.pause_time()
wwt

# ---- new cell ----
n = 30
TIMESPAN = 100
ra_deg = np.linspace(0, 40, n)
dec_deg = np.zeros(n)
unix_times = t0.unix + np.linspace(0, TIMESPAN, n)
iso_times = [Time(ut, format='unix').isot for ut in unix_times]
table = Table({'ra': ra_deg, 'dec': dec_deg, 'time': iso_times})
layer = wwt.layers.add_data_layer(
    table,
    lon_att = 'ra', lat_att = 'dec', lon_unit = u.deg,
    time_att = 'time', time_series = True,
    size_scale = 60
)

# ---- new cell ----
wwt.set_current_time(t0)
wwt.play_time()

We could reduce these issues by wiring up something to change baseDate to be (e.g.) the mean date of the table, or something like that. While this isn't an issue I want to spend too much effort on, it did come up in literally the first manual example I tried, so it seems like something that should at least be documented. I'll ponder a bit more ... anyone else have thoughts?

The previous time implementation had a bug where `set_current_time()` was off
by a month, because JavaScript has the *amazing* thing where the constructor
`Date(year, month, day)` uses zero-based month numbers, but 1-based everything
else. This PR uses a different approach that should fix this issue, but that
has the side effect of changing the images generated during the CI process.
Make the problem go away by altering the reference date to be 2017 Feb 1,
which was not what was intended before, but was what was actually being used.
@astrojonathan
Copy link
Member

The mean data would definitely be better than the lower date especially if we can remove outliers that are effectively null dates.

@pkgw pkgw dismissed astrofrog’s stale review September 24, 2019 01:10

Changes addressed

Copy link
Contributor

@pkgw pkgw left a comment

Choose a reason for hiding this comment

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

OK, there are definitely things to tighten up here — including writing up docs and adding test coverage — but the core functionality seems to be working, and this PR is getting a little unwieldy. So I'm going to make the call to merge it and do the tightening-up in separate follow-up PRs.

@pkgw pkgw merged commit 455fe08 into WorldWideTelescope:master Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants