Skip to content

Conversation

@M-A-Demir
Copy link
Contributor

@M-A-Demir M-A-Demir commented Oct 23, 2024

Added TiffStackReader.ipynb how-to to the '1_Read_and_visualise' folder.
Added 2_Geometry/CreateCustomGeometry.ipynb
Added 3_Processors/FlatDarkFieldNormaliser.ipynb

Closes #170
Closes #173

@M-A-Demir
Copy link
Contributor Author

Potential issue: TIFFStackReader.ipynb downloads the sandstone dataset to its directory (how-tos/1_Read_and_visualise), CreateCustomGeometry.ipynb uses the already-downloaded dataset, but file path was not updated in the notebook.

Where should this dataset be downloaded to in this repo, so I can adjust the file path variables accordingly?

Copy link
Contributor

@hrobarts hrobarts left a comment

Choose a reason for hiding this comment

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

Hi Mariam, these are looking really great - I think they're all very clear. I couldn't add comments on the code for the TIFFStackReader and FlatDarkFieldNormaliser so putting some here:

  • In the TIFFStackReader you could say it accepts .tiff and ,tif files
  • In the FlatDarkFieldNormaliser you could use show2D([sandstone, sandstone_norm]) to show the plots next to each other, you could add titles as well. Maybe the same for the flat and dark plots
  • I think the %store looks like a good way to get the previously processed data but we should discuss how that would work if we're going to use GHA to render these

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this simple geometry example will be really useful for people. I wonder if we should have a separate 2D cone beam, 2D parallel beam, 3D cone beam and 3D parallel beam example?

@MargaretDuff
Copy link
Member

MargaretDuff commented Oct 31, 2024

Hi @M-A-Demir, thanks for your hard work on this! I think all the code is there but there are some places where we could improve the explanations.

  • In the FlatDarkFieldNormaliser notebook, it would be great if you could explain at the top of the notebook what a flat and dark field is and why you would correct for it.
  • In the CreateCustomGeometry notebook, where did you get the SrcToObject and SrcToDetector values from? It might be worth giving a little more explanation of where you found these but mentioning that it will be different for different systems and ways of saving data.
  • In the TiffStackReader notebook, you create a list of tiff_files excluding some of the BBii_????.tif files. Perhaps point to the normalisation notebook with your explanation as to why?

I will leave a few more specific comments in the notebooks

"cell_type": "markdown",
"metadata": {},
"source": [
"The `AcquisitionGeometry` and loaded data are needed to use CIL's visualisation and reconstruction tools. \n",
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this sentence makes sense

"cell_type": "markdown",
"metadata": {},
"source": [
"Using `show2D()`, we can view a central projection of the data:"
Copy link
Member

Choose a reason for hiding this comment

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

Might also be worth showing an FBP reconstruction, to demonstrate that the geometry is correct and thus gives a reasonable reconstruction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Margaret! I added the clarifications you suggested, as well as the reconstruction (& CoR adjustment) to the geometry notebook

…oks. Added extra section for reconstructing and adjusting CoR in Geometry notebook
Copy link
Member

@MargaretDuff MargaretDuff left a comment

Choose a reason for hiding this comment

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

Sorry Mariam, I made a few suggested changes and then between vscode and github they all got deleted. Will try again another time

Copy link
Member

@MargaretDuff MargaretDuff left a comment

Choose a reason for hiding this comment

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

Thanks @M-A-Demir. The code is all there. I have made some suggestions to improve the readability!

"metadata": {},
"outputs": [],
"source": [
"%store sandstone"
Copy link
Member

Choose a reason for hiding this comment

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

What does this do? Generally users will take notebooks verbatim and try and adapt them to their dataset so if this is a cell particular to the how-tos and a not general CIL recommendation then we should state that.

"metadata": {},
"outputs": [],
"source": [
"%store -r sandstone\n",
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean you have to run the previous how-to first. This needs to be made clear if so!

M-A-Demir and others added 8 commits November 14, 2024 13:22
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
M-A-Demir and others added 7 commits November 19, 2024 10:22
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
@M-A-Demir M-A-Demir requested review from hrobarts and removed request for hrobarts and lauramurgatroyd November 19, 2024 10:44
@hrobarts
Copy link
Contributor

Hi Mariam, these are all looking great. I'd be happy to go ahead and merge the TiffStackReader and Normaliser notebooks. I think we should add a simulated example to the Normaliser in future but this works well as a real example.
I would maybe ask @gfardell her opinion on the geometry notebook

Copy link
Member

@MargaretDuff MargaretDuff left a comment

Choose a reason for hiding this comment

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

Looking good @M-A-Demir, have left a few style comments

Comment on lines +78 to +79
"# %store -r sandstone\n",
"# show2D(sandstone)"
Copy link
Member

Choose a reason for hiding this comment

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

Are these meant to be commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have added clarification to the comment above this cell that these should be uncommented if you run and save data from the Geometry notebook

"cell_type": "markdown",
"metadata": {},
"source": [
"The `%store -r sandstone` command allows us to use the `sandstone` `AcquisitionData` we saved in the `1_Read_and_visualise/TIFFStackReader.ipynb` notebook. Please run the TIFFStackReader notebook (without deleting the `data/sandstone` folder), and save the `sandstone` variable before running the cell below."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"The `%store -r sandstone` command allows us to use the `sandstone` `AcquisitionData` we saved in the `1_Read_and_visualise/TIFFStackReader.ipynb` notebook. Please run the TIFFStackReader notebook (without deleting the `data/sandstone` folder), and save the `sandstone` variable before running the cell below."
"Note: The `%store -r sandstone` command allows us to use the `sandstone` `AcquisitionData` we saved in the `1_Read_and_visualise/TIFFStackReader.ipynb` notebook. Please run the TIFFStackReader notebook (without deleting the `data/sandstone` folder), and save the `sandstone` variable before running the cell below."

M-A-Demir and others added 7 commits January 17, 2025 15:14
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Added clarification on commented %store line
Formatting in changed print statements caused notebook failure to load
@gfardell gfardell moved this from In Review to Blocked in CIL work Mar 4, 2025
@M-A-Demir M-A-Demir requested a review from MargaretDuff March 7, 2025 11:13
@hrobarts hrobarts removed this from CIL work Apr 1, 2025
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.

How to prepare data: Normaliser How-to load data and create an AcquisitionData: TiffReader

4 participants