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

Update plots #76

Merged
merged 17 commits into from
May 25, 2021
Merged

Update plots #76

merged 17 commits into from
May 25, 2021

Conversation

jmarshrossney
Copy link
Collaborator

  • Made a few changes to existing plots to make them easier to read
  • Improved the fitting of cosh(...) to the correlator
    • can choose how many small-separation points to ignore in the fit
    • errors are now from a bootstrap, not the covariance of the parameter estimate

Did not:

  • implement layer-wise plotting: we will probably want to discuss how best to do that, whereas I'm hoping this PR can be merged pretty quickly.

anvil/observables.py Outdated Show resolved Hide resolved
raise ConfigError("Seed is outside of appropriate range: [0, 2 ** 32]")
return manual_bootstrap_seed

def parse_cosh_fit_min_separation(self, n: int, training_geometry):
Copy link
Owner

Choose a reason for hiding this comment

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

can this not just take the lattice_length directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, good point

Copy link
Collaborator Author

@jmarshrossney jmarshrossney May 20, 2021

Choose a reason for hiding this comment

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

I can replace training_geometry.length with lattice_length in this parse function, but not the production rule directly below. I'm sure by now I should understand this behaviour but alas, I forget.

Also, since we're moving towards removing training_context (which contains the entire training runcard) in favour of things more akin to training_geometry which just extract the specific parameters we need (#62), perhaps it does make sense to leave this as is.

Copy link
Owner

Choose a reason for hiding this comment

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

that's really weird, I don't understand that..

Copy link
Owner

Choose a reason for hiding this comment

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

I guess leave it for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

apologies, it doesn't work with either the parse or produce - the parse just worked because I was using the default value for cosh_fit_min_separation in cosh_fit_window..

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's because that key is only present in the training context and so we could in theory put lattice length but we'd have to collect over the fit or write the key in the sampling runcard, idk why I didn't think about this earlier.

So definitely leave it for now and I will look into extracting parameters from the trained model in a more satisfactory way..

Copy link
Owner

Choose a reason for hiding this comment

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

aka the issue you linked.

anvil/observables.py Outdated Show resolved Hide resolved
@@ -111,7 +119,7 @@ def magnetic_susceptibility(magnetization, abs_magnetization_squared):


def magnetization_series(configs):
return configs.sum(axis=1)
return configs.sum(axis=1).numpy()
Copy link
Owner

Choose a reason for hiding this comment

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

would it make more sense to just cast the configs numpy earlier instead of multiple times on the observables?

Copy link
Owner

Choose a reason for hiding this comment

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

in other words, do we ever need configs to be a tensor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah it would. I mean I think it only happens the once if you discount the unused correlator calculation, but it would probably be better to cast to numpy at the end of the sampling.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd rather just leave this niggle until we're messing around inside sample.py anyway, trying to generate configurations from each layer, if that's alright.

@@ -5,15 +5,18 @@
{@table_two_point_scalars@}
## Two Point Correlator
{@plot_two_point_correlator@}
{@plot_two_point_correlator_error@}
Copy link
Owner

Choose a reason for hiding this comment

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

do we want to keep adding to this report, or at some point provide some more examples of individual actions or smaller reports?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I have opened an issue which is almost the same as addressing this #78

anvil/observables.py Outdated Show resolved Hide resolved
@wilsonmr wilsonmr merged commit 0c3b496 into master May 25, 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