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

Re-disable buggy check #576

Closed
wants to merge 1 commit into from

Conversation

lucas-wilkins
Copy link
Contributor

Previous attempts to unbug the orientation viewer didn't work out, this re-disables the breaking check until the right solution can be found.

@pkienzle
Copy link
Contributor

I would rather not silently ignore incorrect parameters to the model evaluator. If the caller can't be fixed for this release then throw in a deprecation notice that undefined parameters will raise an error in a future release.

@lucas-wilkins
Copy link
Contributor Author

@pkienzle It's not ideal to disable checks, but was less ideal to merge something that broke the current sasview.

If you could tell me what the new way of doing things is, then it would be pretty easy to update on the sasview end.

The current version (in the branch where the updated parameter names) is something like:

model_info = load_model_info("parallelepiped")
model = build_model(model_info)
...
calculator = DirectModel(data, model)
calculator(
  length_a=a,
  length_b=b,
  length_c=c,
  ... [other stuff])

Using length_a instead of a stops the errors, but it also stops it from calculating anything at all, just returning a uniform field. If you know what's going on, let me know and I'll fix it, if you don't know, then maybe its best to disable the check until we can figure it out.

@pkienzle
Copy link
Contributor

You have a teeny object of size [0.1 x 0.4 x 1.0] which leads to scattering < 3.6e-5 over your q mesh. You are adding a background of exp(-3) ≈ 0.05 then converting to a log-scaled colour mapped over [exp(-3), exp(10)] ≈ [0.05, 22000].

Multiplying your dimensions by 200 works pretty well, giving interesting patterns in the q range and peaks ≈280.

@lucas-wilkins
Copy link
Contributor Author

I'm pretty sure they're the same values as in jitter.py, no? And it worked fine before. Is there something that has changed with the parameter scaling, as well as renaming them all?

@pkienzle
Copy link
Contributor

I never tied the shape parameters to the drawn box size in jitter, hence the confusion.

A simple solution is to divide each dimension of the drawn shape by the max dimension of the calculated shape so the rectangle is drawn with unit length. That way you preserve the aspect ratio without drawing anything too large or too small.

@lucas-wilkins
Copy link
Contributor Author

I think the default parameters just happened to be at least approximately the right shape for the lengths in the orientation viewer. I'll add something that takes a size in Angstroms, and normalises it to 1 along its longest length for the drawing part.

@wpotrzebowski
Copy link

@lucas-wilkins do we still need it? The orientation viewer seems to be working now.

@lucas-wilkins
Copy link
Contributor Author

lucas-wilkins commented Sep 12, 2023 via email

@wpotrzebowski wpotrzebowski deleted the 573-changes-in-563-break-sasview-main branch September 12, 2023 13:27
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.

3 participants