Skip to content

Conversation

@mhitzem
Copy link
Contributor

@mhitzem mhitzem commented Oct 18, 2021

This filter takes a 3D grayscale data array and normalizes the contrast and brightness values on each layer.

@imikejackson
Copy link
Member

Code Review

Generally the code was thought out well. The major hindrance being trying to find the API documentation for SIMPL/DREAM3D. This is still an outstanding issue for the developers @imikejackson @JDuffeyBQ

When writing your code, variable naming is very important. Be specific with variable names. If the variable refers to a DataArrayPath then it should probably have "path" somewhere in the name. There are 2 types of Arrays in the code: SIMPL.DataArra and Numpy array. I would prefix your variable names with np_ or npv_ (numpy view) so that it is easily understood what the variables are holding. I sprinkled some typing information into the code also.

I dug up a long "switch()" for each of the data types so that the proper kind of output SIMPL.DataArray is created. There is a module called simpl_helpers that had the basics. I had to adjust it a bit for this case.

I also moved from using PyCharm to using VSCode with it's built in Pylance which I found gave superior code completion to PyCharm. This might help you navigate the generated python bindings.

As mentioned in the initial meeting, you may want to have the SIMPL source code cloned onto your system as that will help you try to figure out the python bindings. There are numerous unit tests that cover some of the bindings but not all.

As @mgroeber mentioned, the dataCheck() is there is ensure that they values passed into the filter are sane. Just be deatiled in your approach and the code should turn out OK.

Algorithm Results

I do not get the correct results from a few different data sets that I have tried so I think there is an issue with the actual algorithm itself so that will need to be worked out.

Documentation

After this is all said and done, please write a markdown file following the last format. You are free to use the Small IN100 images in the document as an example of before and after images.

@imikejackson imikejackson force-pushed the develop branch 5 times, most recently from f04f5d9 to ae705b8 Compare August 17, 2022 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants