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 3_Multichannel #160

Merged
merged 3 commits into from
Oct 16, 2024
Merged

update 3_Multichannel #160

merged 3 commits into from
Oct 16, 2024

Conversation

casperdcl
Copy link
Member

@casperdcl casperdcl commented Aug 13, 2024

- part of #155
@casperdcl casperdcl self-assigned this Aug 13, 2024
@casperdcl casperdcl added the documentation Improvements or additions to documentation label Aug 13, 2024
@casperdcl casperdcl marked this pull request as ready for review August 13, 2024 18:49
@MargaretDuff
Copy link
Member

MargaretDuff commented Aug 23, 2024

We aren't always seeing progress bars e.g.folder 3 notebook 1:
image

@MargaretDuff
Copy link
Member

Can we zoom this in on folder 3 notebook 3:
image

@MargaretDuff
Copy link
Member

In folder 2 notebook 2, is this a regularisation toolkit issue?
image

Copy link
Member

@lauramurgatroyd lauramurgatroyd left a comment

Choose a reason for hiding this comment

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

Sorry I only got part way through reviewing so far, will continue next week.

"show2D( data, slice_list=[('channel',0),('channel',1),('channel',2)], \\\n",
" title=[\"Red\",\"Green\",\"Blue\"], origin=\"upper\", num_cols=3)"
"show2D(data, slice_list=[('channel',0),('channel',1),('channel',2)],\n",
" title=[\"Red\",\"Green\",\"Blue\"], origin=\"upper\", num_cols=3);"
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
" title=[\"Red\",\"Green\",\"Blue\"], origin=\"upper\", num_cols=3);"
" title=[\"Red\",\"Green\",\"Blue\"], origin=\"upper\", num_cols=3)"

Copy link
Member Author

Choose a reason for hiding this comment

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

definitely not. See OP.

"metadata": {},
"outputs": [],
"source": [
"show_geometry(ag)"
"show_geometry(ag);"
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
"show_geometry(ag);"
"show_geometry(ag)"

Copy link
Member Author

Choose a reason for hiding this comment

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

definitely not. See OP.

@@ -282,7 +277,7 @@
"metadata": {},
"outputs": [],
"source": [
"show_geometry(ag, view_distance = 4.5, elevation=10, azimuthal=-65, figsize=(10,10))"
"show_geometry(ag, view_distance = 4.5, elevation=10, azimuthal=-65, figsize=(10,10));"
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
"show_geometry(ag, view_distance = 4.5, elevation=10, azimuthal=-65, figsize=(10,10));"
"show_geometry(ag, view_distance = 4.5, elevation=10, azimuthal=-65, figsize=(10,10))"

Copy link
Member Author

Choose a reason for hiding this comment

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

definitely not. See OP.

Copy link
Member

@lauramurgatroyd lauramurgatroyd left a comment

Choose a reason for hiding this comment

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

There are quite a few places where check_convergence is called - why is this?

Whilst checking, I found some additional issues with the notebooks which need updating, sorry

demos/3_Multichannel/02_Dynamic_CT.ipynb Outdated Show resolved Hide resolved
demos/3_Multichannel/02_Dynamic_CT.ipynb Outdated Show resolved Hide resolved
demos/3_Multichannel/02_Dynamic_CT.ipynb Outdated Show resolved Hide resolved
demos/3_Multichannel/02_Dynamic_CT.ipynb Show resolved Hide resolved
" [(\"channel\", 20),(\"horizontal_y\", 35)],\n",
" [(\"channel\", 20),(\"horizontal_x\", 25)]],\n",
" cmap=\"inferno\", num_cols=3,\n",
" origin=\"upper\", fix_range=(0,0.5));"
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
" origin=\"upper\", fix_range=(0,0.5));"
" origin=\"upper\", fix_range=(0,0.5))"

Copy link
Member Author

Choose a reason for hiding this comment

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

definitely not. See OP.

demos/3_Multichannel/03_Hyperspectral_reconstruction.ipynb Outdated Show resolved Hide resolved
demos/3_Multichannel/03_Hyperspectral_reconstruction.ipynb Outdated Show resolved Hide resolved
@lauramurgatroyd
Copy link
Member

Sorry if I've missed this but why do we need ';' after show2D if its the last cell line?

@lauramurgatroyd
Copy link
Member

Can we zoom this in on folder 3 notebook 3: image

image

This was the biggest I could get it, but it removes the tilt which possibly makes it less useful.

I think there are some issues with show_geometry which make it difficult to zoom in - I will investigate further and open an issue in CIL

@lauramurgatroyd
Copy link
Member

I am going to revert the show_geometry change

@lauramurgatroyd
Copy link
Member

Relevant issue in CIL: TomographicImaging/CIL#1954

Copy link
Member

@lauramurgatroyd lauramurgatroyd left a comment

Choose a reason for hiding this comment

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

I have now made the changes that I suggested. @MargaretDuff would you like to review or shall I merge?

demos/3_Multichannel/02_Dynamic_CT.ipynb Show resolved Hide resolved
demos/3_Multichannel/02_Dynamic_CT.ipynb Show resolved Hide resolved
@MargaretDuff
Copy link
Member

image
Needs an additional: as_array()

@MargaretDuff
Copy link
Member

Tested on 24.2.0 - happy to merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants