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.

Sign up for GitHub

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

Jump to bottom

Z range of cropped images not set by config file #596

Closed
derollins opened this issue Jun 2, 2023 · 11 comments · Fixed by #597
Closed

Z range of cropped images not set by config file #596

derollins opened this issue Jun 2, 2023 · 11 comments · Fixed by #597
Assignees
@SylviaWhittle
Labels
enhancement New feature or request

Comments

@derollins
Copy link
Collaborator

Currently the z scale of the cropped grains that are generated in the all image set is relative, set between the highest and lowest points of the image rather than by the config file.

It would be better if the z scale of these cropped images could also be set by the z range in the config file.
@derollins derollins added the enhancement New feature or request label Jun 2, 2023
@SylviaWhittle SylviaWhittle self-assigned this Jun 2, 2023
@SylviaWhittle SylviaWhittle mentioned this issue Jun 2, 2023
Merged
@ns-rse
Copy link
Collaborator

ns-rse commented Jun 3, 2023

Would it perhaps be better if the z-scale of cropped grains is set relative to the grain itself rather than the overall image or user specified configuration values?

It seems to me that whilst allowing the user to specify the range they want to get things perfect for a specific grain out of many that might have been detected this is harking back to the perennial issue of plotting being something that is so variable that we can not realistically accommodate all eventualities in an automated workflow (which is what TopoStats is meant to be). Thus making all plots of individual grains as specific as possible seems the most general solution because any user specified values might work for one grain, but not another.

The suggestion of using values from a configuration file means that a cyclical process would be entered where an initial run is made, the results are not quite right and so values are tweaked and another run made, repeat this until the desired image is spat out.

This is a sub-optimal approach to an automated work-flow which should give general, albeit rough, results that are usable. When users then want something more bespoke they should be able to take the processed data and plot it as desired.

If the alternative I'm proposing is desired then saving not just the whole image but the cropped grains as .npy (NumPy) arrays would greatly facilitate this.

@SylviaWhittle
Copy link
Collaborator

Thanks @ns-rse. Those are some good points. I am unsure about what is the right philosophy to take here, however I would like to mention that when using TopoStats, the previous grain plotting has often been near useless for showing the grains, as high points in the image often dominate, causing the image to look either all black with a few bright pixels, or all bright and the background is black, so it's hard to see the sample surface. The solution shown in the pull request seems to solve this issue, but as you point out, could be introducing other issues.

@ns-rse
Copy link
Collaborator

ns-rse commented Jun 4, 2023

Wouldn't using the range for cropped grains help in this instance though? Often the high points are specific to individual grains and there may be one piece of "noise" that is detected as a grain with extreme values for the height. When the unit of processing switches to being a grain and the z-range is specific to that individual grain it then doesn't matter about the heights of other grains (genuine or noise).

@alicepyne
Copy link
Member

alicepyne commented Jun 4, 2023 via email

@ns-rse
Copy link
Collaborator

ns-rse commented Jun 4, 2023

Its still two runs to get that though, one to process initially find out what the heights are without any noise, go back, tweak the configuration file to the desired z-range and re-run.

If noise can be removed and the maximum height determined across the true grains of interest then that value could be used on the first run without requiring the user to explicitly specify these values. The challenge is making sure all "noise" with excessive heights are removed first. Some of this isn't happening until dnatracing (the infamous purge_obvious_crap() where "blobs" when skeletonised are reduced to skeletons < 10 pixels).

To this end I think improved modularisation would go some way to improving things, as I'm going through dnatracing refactoring I'm intending on using the dictionary of grains that grainstats produces, currently this isn't used and they are derived anew.

As it stands grain plotting is buried within GrainStats() class and it perhaps shouldn't be. If we take that out and have plotting as a separate stage using the returned dictionary of grains it becomes more flexible as we can remove those thrown out during dnatracing, assess the maximum height of true grains based on the data without the need for user input and use that when plotting the remaining grains of interest.

The proposed fix gets the desired functionality quickly but at the expense of a convoluted user workflow. I'm trying to piece together the bigger picture and make the code and processing steps more modular to avoid this repetition.

@SylviaWhittle
Copy link
Collaborator

(I think plotting for grains is not in GrainStats(), as a dictionary containing the image subsections are returned to process_scan() which then plots the grains. The only plotting that happens inside grainstats is some debug junk left in from my development of it, and probably should be removed given that as far as I know nobody has used it ever.)

Just for context in this discussion, here are two grain images produced in the current TopoStats that have differing height scales:

image
image
image

and here are the grains when the z-scale is applied
image
image
image

(just noticed that the grains are cropped badly, will look into that)

@alicepyne
Copy link
Member

alicepyne commented Jun 4, 2023 via email

@ns-rse
Copy link
Collaborator

ns-rse commented Jun 5, 2023 •
edited
Loading

I think plotting for grains is not in GrainStats(), as a dictionary containing the image subsections are returned to process_scan() which then plots the grains.

Apologies, my mistake and I should have checked (it was Sunday though so I opted not to delve deep).

I do think the proposed #517 will help with workflow here and have had some thoughts on how to proceed with developing that along with all the other changes that are happening.

EDIT : Just checked and plotting does indeed follow in process_scan() (lines 236-248 of topostats/processing.py).

There is also the request to exclude grains from masking based on height #431 (#574) and perhaps the restriction of z-range for plotting could be linked to this in some manner?

It might help reduce the number of options in the configuration file which users have already reported is starting to get overwhelming.

@derollins
Copy link
Collaborator Author

derollins commented Jun 5, 2023 •
edited
Loading

Thanks for thinking through this and the implications on the rest of the workflow.

I don't know much of the broader plans for developing the code but I think it makes sense from my own use that the z scale for the cropped images should match the the user defined z scale in the config file. Since the user has already made a judgement about the best z scale for the image by setting it for the full scan image so the features of interest are clear it make sense to use that scale in the cropped images as this will generate a library of individual grains that can be directly compared with each other which I believe is the original purpose of the cropped grains and certainly what I use them for.

In the case that the user doesn't define a z scale (leaves z range as [null, null]) then using the high and low values for each individual image makes sense since the features of interest might not yet be determined and it is best to display each grain in its own range. This is how the cropped grains are currently scaled I belive.

Another related but separate issue is that the usual (default?) colour scale we use is 'Nanoscope' is non-linear with much clearer definition in the higher range which means images scaled from minimum to maximum value often don't display very well anyway.

@ns-rse
Copy link
Collaborator

ns-rse commented Jun 5, 2023 •
edited
Loading

I've just had a look at a single .spm file and it contains the fields \Z min: and \Z max:....

grep: warning: stray \ before Z
\Fast cal freq: 2.50401
\Slow cal freq: 4.89064
\Xs-Yf coupling: 0.00170358
\Xs-Yf coupling derating: -1.23204e-05
\Ys-Xf coupling: -0.0144994
\Ys-Xf coupling derating: -3.29532e-05
\X offset sens: 94
\Y offset sens: 91
\Max scan size: 40
\Z min: -220
\Z max: 220
\Z engage position: 0
\Z to X coupling: 0
\Z to Y coupling: 5
\StrainXY Scanner: Yes
\OptoXY x sensitivity: 190.5
\OptoXY y sensitivity: 203.4
\OptoXY orthogonality: 0
\OptoXY x sens fitted: 110
\OptoXY y sens fitted: 120
\OptoXY orthog fitted: 0

Would these be the values the user has specified when making the scan?

If so that would provide a very convenient method of obtaining the value and save growing the configuration file. These could be used both for scaling and for excluding grains based on height as per #431 (/#574).

@alicepyne
Copy link
Member

alicepyne commented Jun 5, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@SylviaWhittle SylviaWhittle

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Sylvia whittle/596 zscale grains AFM-SPM/TopoStats
4 participants
@alicepyne @ns-rse @derollins @SylviaWhittle