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

Convergence diagnostics #55

Merged
merged 65 commits into from
Feb 21, 2020
Merged

Convergence diagnostics #55

merged 65 commits into from
Feb 21, 2020

Conversation

jeffjennings
Copy link
Collaborator

@jeffjennings jeffjennings commented Feb 20, 2020

Add plotting for convergence diagnostics, other various minor changes

Fix #45 Fix #46 Fix #47 Fix #49 Fix #50

@jeffjennings: the "Fix #N" statements I added yesterday in this description were not reminders to be removed.

A "Fix #N" in a PR description or in a commit message will close the #N issue when the PR is merged. It is a bad practice to close the issues manually, and it is actually better not to unless they become irrelevant, and won't imply any change to the repo.
Using the "Fix #N" statements ensures that the issues are closed really when they are closed (i.e. when the PR is merged). Until this PR is merged the issues are not closed. If we were not to merge this PR now, you would have closed issues that are not actually closed, with the obvious confusion.
Secondly, by using "Fix #N" statements Github automatically adds a reference with a link to the PR or the commit where the issue was closed, in the issue page, which helps a lot debugging later on. While now you added manually "see file..." without the PR reference number, which makes it very hard to track the commit down

@rbooth200
Copy link
Collaborator

This PR is ready to go from my point of view.

@rbooth200
Copy link
Collaborator

Marco tells me @jeffjennings is still working on this, I'll wait to close it.

@rbooth200
Copy link
Collaborator

rbooth200 commented Feb 20, 2020

I've fixed some issues that pylint was giving, but the diagnostic plots aren't working yet, apparently.

@rbooth200
Copy link
Collaborator

Quick Q: why did you remove the pylint directives?

@jeffjennings
Copy link
Collaborator Author

jeffjennings commented Feb 21, 2020

My mistake, I thought it had automatically added those and that they could be removed. I've just pushed with them added back in.

@jeffjennings
Copy link
Collaborator Author

Everything works for me on this branch now, including the diagnostic plot.

@rbooth200
Copy link
Collaborator

rbooth200 commented Feb 21, 2020

I'd also suggest replacing start_iter and stop_iter with a single num_iter parameter and just print the last so many iterations. Isn't that what will be wanted anyway? I think there is too much chance of error with the current parameters.

Obviously if you ask for 1000 and it converged in 20 then we just show all of them.

@rbooth200
Copy link
Collaborator

The pylint directives are to stop it reporting false positives

@jeffjennings
Copy link
Collaborator Author

Hey @mtazzari, @rbooth200 and I have both been contributing to this branch that closes the outstanding issues in the Code freeze milestone. Can you review and merge into master? Thanks!

@mtazzari
Copy link
Collaborator

mtazzari commented Feb 21, 2020

Ok, starting the review

@rbooth200
Copy link
Collaborator

I've just merged in some changes needed to get tests of the full pipeline working

Copy link
Collaborator

@mtazzari mtazzari left a comment

Choose a reason for hiding this comment

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

Some small changes requested.
Ok to merge after that.

docs/install.rst Outdated Show resolved Hide resolved
docs/quickstart.rst Outdated Show resolved Hide resolved
docs/quickstart.rst Outdated Show resolved Hide resolved
docs/quickstart.rst Outdated Show resolved Hide resolved
frank/geometry.py Show resolved Hide resolved
frank/radial_fitters.py Show resolved Hide resolved
jeffjennings and others added 4 commits February 21, 2020 16:26
Co-Authored-By: Marco Tazzari <mtazzari@users.noreply.github.com>
Co-Authored-By: Marco Tazzari <mtazzari@users.noreply.github.com>
Co-Authored-By: Marco Tazzari <mtazzari@users.noreply.github.com>
Co-Authored-By: Marco Tazzari <mtazzari@users.noreply.github.com>
@jeffjennings jeffjennings merged commit 9471c19 into master Feb 21, 2020
@jeffjennings jeffjennings deleted the convergence_diagnostics branch February 21, 2020 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants