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

139 prepare for magnetism refl1d #140

Merged
merged 6 commits into from
May 2, 2024
Merged

Conversation

andped10
Copy link
Collaborator

@andped10 andped10 commented Apr 29, 2024

Move functionality used more than 1 time to private functions:

  • _get_probe
  • _get_polarized_probe
  • _get_oversampling
  • _get_sample

Made it possible to do a POC calculations with magnetism by setting MAGNETISM=True

Renamed constants to make it more clear what they represents.

Introduced new polarized probe that is needed for calculations with magnetism. The results obtained from the "ordinary" probe and the polarized probe are not identical. No work have been done yet to understand these differences.

Removed functionality that is no longer used:

  • 'resolution': 'dq' and dq in refnx
  • update_item in refld

@andped10 andped10 added the feature New functionality or request label Apr 29, 2024
sample = _build_sample(self.storage, model_name)
probe = _get_probe(
q_array=np.linspace(0.001, 0.3, 10),
dq_array=np.linspace(0.001, 0.3, 10), # TODO why would we use a steadily increasing dq (resolution)?
Copy link
Member

Choose a reason for hiding this comment

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

Where do these numbers come from?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally speaking, the numbers are a sensible range o be calculating reflectivity over. I am less sure about the linspace though as logspace is used in alot of real experiments. Though it doesn't really matter so much

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Realized that the provided q and dq are not used for anything in the SLD determination. However, they are required and are now changed to 1 as a dummy value and a code comment has been added.

Copy link
Member

@rozyczko rozyczko left a comment

Choose a reason for hiding this comment

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

minor comments about coding style

EasyReflectometry/calculators/refl1d/wrapper.py Outdated Show resolved Hide resolved
EasyReflectometry/calculators/refl1d/wrapper.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jfkcooper jfkcooper left a comment

Choose a reason for hiding this comment

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

A few comments, some of which are almost certainly due to my inability to understand refl1d's code documentation - so apologies for those ones

sample = _build_sample(self.storage, model_name)
probe = _get_probe(
q_array=np.linspace(0.001, 0.3, 10),
dq_array=np.linspace(0.001, 0.3, 10), # TODO why would we use a steadily increasing dq (resolution)?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally speaking, the numbers are a sensible range o be calculating reflectivity over. I am less sure about the linspace though as logspace is used in alot of real experiments. Though it doesn't really matter so much

@andped10 andped10 requested a review from jfkcooper May 2, 2024 06:20
@andped10 andped10 marked this pull request as ready for review May 2, 2024 06:21
Copy link
Collaborator

@jfkcooper jfkcooper left a comment

Choose a reason for hiding this comment

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

My only comment is about the equality checking, which you can do with as you like

Copy link
Member

@rozyczko rozyczko left a comment

Choose a reason for hiding this comment

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

happy with code changes so approving.

@andped10 andped10 merged commit b6cbc57 into develop May 2, 2024
26 checks passed
@andped10 andped10 deleted the 139-prepare-for-magnetism-refl1d branch May 2, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants