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

Problems with pars in notebooks #329

Open
jadball opened this issue Sep 27, 2024 · 12 comments
Open

Problems with pars in notebooks #329

jadball opened this issue Sep 27, 2024 · 12 comments

Comments

@jadball
Copy link
Contributor

jadball commented Sep 27, 2024

Hi James,

Just to give you some feedbacks about your very new updates on the notebook.

Seen with user measurement data, found two small issues:

  1. in the indexing notebook, we have to do:

cf_4d.parameters.loadparameters(par_file)

cf_4d.updateGeometry()

before ucell = ImageD11.unitcell.unitcell_from_parameters(cf_4d.parameters)

It seems cf_4d = ds.get_cf_4d() does not get parameters. Probably this is because ds does not contain parameters any more.

  1. in the sinogram_map notebook:

write_h5(ds.grainsfile, grainsinos, overwrite_grains=False) currently has a different key from the old version which uses "write_grains_too" ?

So I guess the notebook needs to be updated to the new key name.

Cheers,

Haixing

Originally posted by @haixing0a in 3eb8107

@jadball
Copy link
Contributor Author

jadball commented Sep 27, 2024

@haixing0a can you please confirm that you're using the latest notebooks from git master?
I'm pretty confident that I tested these things before merging.
When you run the following lines, the cf_4d should have the correct parameters assigned:

cf_4d = ds.get_cf_4d()
ds.update_colfile_pars(cf_4d, phase_name=phase_str)

@jonwright
Copy link
Member

I broke that. Not sure if I remember why?

@jonwright
Copy link
Member

Or it might not be me - I can't read git history. Looks like this one:

c066ed7#diff-4b57e38df8ad163dfd1918781b95543a751ccf94e23e9849da31865c626ea033L631

Anyway - please don't revert - when I open the cf_4f from ImageD11.sinograms.properties I want the raw peaksearch output with nothing added. Call dataset.update_colfile_pars or just fix the notebook to load and use the parameters.

@jadball
Copy link
Contributor Author

jadball commented Sep 27, 2024

The current implementation in dataset and the current notebooks should work fine, hence my confusion here. I need more info about how exactly this is breaking to know whether a fix is needed

@jadball
Copy link
Contributor Author

jadball commented Sep 27, 2024

I suspect an older notebook is being used

@jonwright
Copy link
Member

Unrelated, but before I forget: the phase_str should not be used for loading the cf_4d.

https://github.com/FABLE-3DXRD/ImageD11/blob/master/ImageD11/nbGui/S3DXRD/1_S3DXRD_index.ipynb

# [cell ~ 7]
cf_4d = ds.get_cf_4d()
ds.update_colfile_pars(cf_4d, phase_name=phase_str)

@jadball
Copy link
Contributor Author

jadball commented Sep 27, 2024

I'm not sure I know what the problem is here.
When we call ds.get_cf_4d(), we don't touch the parameters at all.

When we call ds.update_colfile_pars(cf_4d, phase_name=phase_str), we are updating the parameters object for cf_4d using pars from disk (via ds.parfile). We specify a phase_str so that we load the correct phase parameters into the parameters object - if phase_str is not given, it will take the first phase from the json file instead.

@haixing0a
Copy link
Contributor

James, sorry, I think I was using a notebook from the version 1.5 week ago which still uses .par for parameters. I realized the the newest notebook needs json file to contain different pars now.

@jadball
Copy link
Contributor Author

jadball commented Sep 27, 2024

@haixing0a no problem, easy to do! I think between the three of us we need to think of better ways of setting up the notebooks for a new experiment - perhaps moving them to a new git repo, and always having a checkout of the latest notebooks somewhere in inhouse that we can copy into SCRIPTS?

@jonwright
Copy link
Member

In general: the peak positions in cf_4d only need to know the detector parameters - then saving the cf_4d will not depend on the phase id.

It will mean adding a unitcell or phase_name argument for select_ring_peaks_by_intensity or indexer_from_colfile, etc. I am thinking the old style parameters inside of the columnfile are not the right place to store the unit cell in the future. We need this for backward compatibility, but it doesn't feel right going forward.

@jadball
Copy link
Contributor Author

jadball commented Sep 27, 2024

It will mean adding a unitcell or phase_name agument for select_ring_peaks_by_intensity or indexer_from_colfile, etc.

This is what I did for now - see 06fae6b

I agree that columnfiles being phase-aware is confusing and messy. Personally I think columnfiles should not have parameters at all, to avoid duplication of parameters objects in memory (where you modify one but forget about the other, etc etc)

@jonwright
Copy link
Member

The reason to put the detector parameters in with the peaks is so that you can have several columnfiles and each one has it's own calibration (e.g. hydra arrangements). Loading one parameters object and then saving a pointer avoids the duplication (e.g. colfile.parameters = shared_parameters ).

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

No branches or pull requests

3 participants