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

Signal1D/Signal2D merge into master #1033

Merged
merged 172 commits into from
Jun 16, 2016

Conversation

dnjohnstone
Copy link
Contributor

Following #963 to 0.8.x this is the merge in to master.... I will finish it by the end of the week.

@dnjohnstone
Copy link
Contributor Author

dnjohnstone commented Jun 14, 2016

Ok - this has become a total mess....

As I understood it the plan was to merge this in to master before anything else to avoid repeated merge frustrations. The day that we decided that this PR was made and was incredibly close except for the single Travis failure that I couldn't track on my machine because it wasn't failing on my machine.

Unfortunately, for some reason, loads of things have now been merged into both 0.8.x and master before the main Signal1D/Signal2D split was made. You'll see that the diff now stands at about 230 files which is going to make this incredibly difficult to review and this problem will only get worse if people keep merging things in to the master. Don't forget that every time you merge one of those PR's every file touched in both 0.8.x and that PR has to be remerged and since that's nearly everything in 0.8.x this creates a huge amount of work and only increases the likelihood of errors.

At this point I think that it might actually be best to start again with a fresh 0.8.x --> master merge because people have changed such a huge amount since this PR was made.

@francisco-dlp
Copy link
Member

Currently tests are not passing due to i) external deprecation warnings ii) internal deprecations warnings. i) could be addressed in a different PR but ii) should be addressed in this one. Happily it seems that the non reproducible error is now gone, so it was probably due to a numpy bug.

The internal deprecation warning seems easy to fix : "The method AxesManager.connect() has been deprecated and will be removed in HyperSpy 0.10. Please use AxesManager.events.indices_changed.connect() instead."

error not raised, but new axes are not properly connected...
@to266
Copy link
Contributor

to266 commented Jun 15, 2016

I've fixed the warning in dnjohnstone#9 however it does not fix the behaviour - the swapped axes do not seem to be correctly connected to the navigation events, and I could not figure it out for now (will try agian tomorrow if noone fixes it by that time)...

@dnjohnstone
Copy link
Contributor Author

@francisco-dlp this is now passing the tests thanks to @to266.

As regards external deprecation warnings - maybe I don't understand them, but it seems like most of them are not our fault? e.g. travis picks up inspect.getargspec which should become inspect.signature, however, we only used this in one place and I've now made that change so I don't see how it can be us.

@francisco-dlp
Copy link
Member

According to @to266 (see post above) the warning issue is now fixed but something is still wrong with the connection. Let's wait to hear back from @to266 tomorrow.

Tomas Ostasevicius and others added 2 commits June 16, 2016 16:28
@to266
Copy link
Contributor

to266 commented Jun 16, 2016

The last issue I had with this is PR'ed here dnjohnstone#10

Once that is merged (edit: it is now!), I suggest someone merges this ASAP. Any leftover bugs will be much easier to fix in separate PRs

@dnjohnstone
Copy link
Contributor Author

Thanks @to266 for the final fix, I really should learn about events better -- if you're happy with this @francisco-dlp I'd also be happy if this got merged.... I've been using this for all my day to day stuff for the past couple of days and haven't hit any bugs that I haven't already fixed.

PS... myself and Tomas did go through plotting tests as well.

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.

None yet

4 participants