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

Fixes for shiny Ska3 #317

Merged
merged 5 commits into from
May 15, 2020
Merged

Fixes for shiny Ska3 #317

merged 5 commits into from
May 15, 2020

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Feb 3, 2020

Description

This has some changes that are required to pass tests in ska3-shiny (Python 3.6 version). Details are given in the commit messages.

This branch passes tests in current ska3-flight. The only catch is that 186897d will be slower in ska3-flight.

  • Passes unit tests on MacOS (ska3-flight)
  • Passes unit tests on MacOS (shiny: astropy 4.0.1, numpy 1.18, etc)
  • Timing regression testing acceptable

Timing

Running proseco/tests/timing.py : time_get_aca_catalog(n_samples=100)

  • ska3-flight : master (4.8.1 @ 52b75ee): 370 ms / catalog
  • ska3-flight : shiny-fix (6dd88ea): 388 msec / catalog (5% slower)
  • shiny : shiny-fix (6dd88ea): 317 msec / catalog (14% faster)

So this is a little slower in current Ska3-flight, but will be faster in shiny.

@jeanconn
Copy link
Contributor

@taldcroft do you want this for the matlab release or don't care?

@taldcroft
Copy link
Member Author

Not for the MATLAB release. Still need to check on performance and this is a lower priority.

@jeanconn
Copy link
Contributor

That's what I figured. So I think proseco, chandra_aca, sparkles are good to have releases cut now.

@taldcroft
Copy link
Member Author

@jeanconn - I believe this is ready now. I've done performance testing and things look OK, see the description.

taldcroft added 5 commits May 13, 2020 06:27
Somewhere between numpy 1.12 and 1.15 the
behavior of argsort changed so that
np.argsort([0] * 20) might not be stable.
Table was relying on that (upstream fix needed),
but in the meantime adding columns like this is
now efficient in astropy Table.
Previously astropy Table was making a deep copy
of meta attributes but this changed for improved
efficiency.
@@ -507,7 +507,8 @@ def plot(self, ax=None, **kwargs):
kwargs.setdefault('bad_stars', np.zeros(len(kwargs['stars']), dtype=bool))

fig = plot_stars(attitude=self.att, ax=ax, **kwargs)
plt.show()
if 'agg' not in plt.get_backend().lower():
plt.show()
Copy link
Member Author

Choose a reason for hiding this comment

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

In shiny (new matplotlib) one gets a warning in tests about calling plt.show() for a non-interactive backend.

Copy link
Contributor

@jeanconn jeanconn left a comment

Choose a reason for hiding this comment

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

This looks good to me. For proseco changes I'm also wondering if trouble will be saved by testing on Windows before merging (I haven't started up that VM in a while but suppose I need to get more facility with it anyway).

@taldcroft taldcroft merged commit 3621584 into master May 15, 2020
@taldcroft taldcroft deleted the shiny-fix branch May 15, 2020 22:15
@taldcroft
Copy link
Member Author

I think that testing individual package updates by hand will not save trouble (i.e. reduce level of effort). Windows-only fails are pretty rare, so until we get to a point where tests are automated or really easy to run, it will be less effort to fix rare problems at the end than always test up front.

Note that despite efforts in that direction, Windows testing is a bit of a pain and we are not testing the exact environment that the FOT will be using anyway.

@javierggt javierggt mentioned this pull request Dec 7, 2020
@javierggt javierggt mentioned this pull request Apr 6, 2021
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.

2 participants