Update notebooks#148
Conversation
Replaced with line plots at various points in time.
And added docstring to animate_1d
Mibitrans would slightly underestimate concentrations if alpha_z is be 0.
To this end, also refactored parameter calculations to split up functionalities in more functions.
|
Minor changes to further documentation
|
JaroCamphuijsen
left a comment
There was a problem hiding this comment.
@JoYvBa I got untill the keesler notebook, will continue later this week, but I thought you might want to look at my comments already
|
|
||
| Parameter used for transport modeling in *mibitrans* package with quantity name, mathematical symbol (used in equations), model input name and default unit. Sorted by parameter type. Quantities marked with a "*" are internally calculated and not defined by the user. | ||
|
|
||
| | Parameter | Symbol | *mibitrans* input | Unit | |
There was a problem hiding this comment.
Would it be an idea to add a column to indicate which parameters are optional and/or have a default value and what that default value is? I think that could be useful information for users.
There was a problem hiding this comment.
Good point, I agree, it would be a nice clarification to have that in one place for overview. I will have to think how to design it to make it both concise and clear, since there are parameters that are conditionally optional (i.e. either v or K and i to get flow velocity, if you do not give v, K and i are not optional).
| @@ -1129,15 +1266,22 @@ | |||
| "metadata": {}, | |||
| "outputs": [], | |||
| "source": [ | |||
| "mb_nd = mbt_nodecay.mass_balance()\n", | |||
There was a problem hiding this comment.
Running this cell gives me the warning that the plume extends beyond the model length, while the text after the next cell, ensures us that this plume actually fits in the model domain. So we should either change the model parameters or the text in line 1293
There was a problem hiding this comment.
The warning was absolutely right, solved by extending the domain a little. It was a leftover from when this warn feature was specifically covered by showing that the original domain does not contain the entire plume.
| "id": "9a8471a2ae8855c3", | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ |
There was a problem hiding this comment.
Whether this type of importing works or not depends on what the current working directory is. I started my notebook inside of the examples folder which made this cell to give an error that the examples module does not exist. Removing the "examples." in each of the lines solved the issue. You could add a line at the start of this notebook to instruct users how this is assumed to be run so that they won't have to figure it out themselves. Maybe also add a comment in the this cell to give a hint about what might be wrong.
There was a problem hiding this comment.
Since the BIOSCREEN_data file only contains three data objects, I would say a better solution is maybe to store and import the data in a different way. Perhaps store them as a json file and import it in the notebook by specifying the path explicitly. Then it is clear to a user, trying the example that they should change that path for their setup / jupyter session.
There was a problem hiding this comment.
When I run the file in the same place, it works fine. Which is weird, since as you point out, examples is the current directory, so importing from examples.BIOSCREEN_data should indeed fail. I am a bit unsure how an user will use this eventually. If they just download the entire examples folder there should be no problem after changing the import statement. But if they just download the individual notebook, it will be unable to locate it of course. Do you mean that the json should be located in mibitrans itself? So you could do from mbt.BIOSCREEN_data import ....? Lets discuss this perhaps next coworking day.
Thanks for the comments @JaroCamphuijsen! I will work on it |
To prevent dependency on TkAgg, animation example now only shows saving output as .gif, not displaying inside IDE. To prevent any import issues when working with the benchmarking_BIOSCREEN example, the bioscreen example data has been relocated to mibitrans.data.example_data, as seperate classes for BIOSCREEN and BIOSCREEN-AT data. With the respective arrays as class properties.
JaroCamphuijsen
left a comment
There was a problem hiding this comment.
Here are again some review comments.
I'll continue tomorrow.
| "id": "a0a0aa9f-22fb-4b33-8726-eafb878eb16d", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "Note that BIOSCREEN only provides limited resolution which causes visual differences in the results, particularly for instantaneous reaction model. Actually calculated values (indicated with markers) are identical." |
There was a problem hiding this comment.
Is it perhaps an idea to remove the dotted line from in between the BIOSCREEN datapoints? That perhaps makes it clear that the data is discrete instead of continuous as opposed to the output of the analytical model.
There was a problem hiding this comment.
I understand what you mean, although simultaneously, I would argue that the segmented dotted lines bring attention to that despite the BIOSCREEN points corresponding with the output curve of the mibitrans model, all data, BIOSCREEN gives no information on how the solution behaves in between those points. In the bioscreen figure for instant reaction, you can see that the start and end of the curve are not very well represented by the implied straight segments of BIOSCREEN. Also, BIOSCREEN is as continuous as mibitrans models when just looking at the equations. Its just that BIOSCREEN has a fixed amount of x, y and t resolution (amount of steps) while mibitrans has no such restriction.
But I agree with the overall point, perhaps we can lower the transparency of the dotted lines a bit to bring more attention to the points for BIOSCREEN.
|
|
||
| The code is structured in a modular fashion to ensure easy extension of functionality by the (future) development team as well as outside collaborators. To stimulate embedding in the scientific community, users are encouraged to report bugs and suggest extensions and issues on the GitHub issue tracker, or even self-implemented features, through pull requests. | ||
|
|
||
| <img src="mibitrans_schematics_structure.png" alt="Overview of `mibitrans` package structure and functionalities." width="500"> |
There was a problem hiding this comment.
For me this image does not show in the local build. I would also suggest to stick to the markdown image format if you can. That said, the mkdocs asset manager is a bit of a pain. I got it working in the end by putting the image in a subdirectory of the implementation directory and using the following image link:
| <img src="mibitrans_schematics_structure.png" alt="Overview of `mibitrans` package structure and functionalities." width="500"> | |
|  |
There was a problem hiding this comment.
I added it to the general assets folder instead and it worked, at least when locally serving the documentation. Is that something that will not work once pushed to GitHub?
|
|
||
| <img src="assets/model_comparison_trans.svg" alt="Comparison between different mibitrans" width="500"> | ||
|
|
||
| <img src="assets/plume_animation.gif" alt="Overview of `mibitrans` package structure and functionalities." width="500"> |
There was a problem hiding this comment.
This image does not exist, also, maybe just include them using the markdown format. I usually really only use the html tag when I need to specify very specific things regarding the formatting. (See my suggestion in implementation.md)
There was a problem hiding this comment.
| <img src="assets/plume_animation.gif" alt="Overview of `mibitrans` package structure and functionalities." width="500"> | |
|  |
There was a problem hiding this comment.
Found the problem, my gitignore has *.gif (and *.png) to prevent any generated figures from escaping to GitHub. But this then applied to the docs as well. I added !docs/**, which should resolve the issue.
Thanks! Looking forward to the rest :)! |
Ensured that all figures in documentation can be displayed. Changed mibitrans schematics to a .svg to preserve resolution. Changed all html figure visualization in docs to markdown format. Implementation is now correctly spelled. Animate plot plots no longer shows static plots in documentation. Automatically generated titles no longer repeat the word 'model'. Made custom titles for some of the plots in example_mibtirans_anatrans.
|
JaroCamphuijsen
left a comment
There was a problem hiding this comment.
There are some minor comments left, that we can also tackle in a future PR if you want. It's quite hard for me to review the scientific correctness of these changes (especially because it is quite a lot!) but I went through everything and at least made sure that everything could run (not everything does, see the model comparison notebook). I think overall, we have a lot of examples now, I think some of the examples could benefit from a little decrease in length, perhaps by selecting the truly interesting comparisons and leaving the ones that are slightly less interesting out.
Overal, nice work! Once the error in the model comparison notebook is fixed, I think we can merge this in.
There was a problem hiding this comment.
@JoYvBa I really like this notebook and all the various comparisons you made between the models implemented in mibitrans and the BIOSCREEN output.
In the plots that compare the mibitrans models to the BIOSCREEN results, I would always just remove the connecting dotted lines, because they connect the datapoints with straight lines it sometimes gives the impression that the data is slightly off, while it is actually spot on. By removing the dotted lines, you force the user to compare the actual location of the dots with the continuous output from mibitrans.@Jorri
| "plt.pcolormesh(\n", | ||
| " mbt_object.x[:len(mbt_object.x)//4],\n", | ||
| " mbt_object.y,\n", | ||
| " diff_mbt_bio[:,:len(mbt_object.x)//4],\n", |
There was a problem hiding this comment.
This line gives me an error as the variable diff_mbt_bio is a single float and cannot be indexed. I think you expect this to be the difference between each of the models, but would that be a mean accross all time of at a certain timepoint? In any case, this variable is set to be the the mean difference accross both spatial dimensions and the time dimension, so it is just a single scalar value.
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "import differences as df" |
There was a problem hiding this comment.
I would try to find a different abbreviation, df is in the context of manipulating arays very easily confused with the df from the pandas "dataframe".
| import mibitrans as mbt | ||
|
|
||
|
|
||
| def mask(a, b, cutoff): |
There was a problem hiding this comment.
Instead of creating a mask and returning the masked arrays on every of the below functions, wouldn't it be better to create masked array's one time in the notebook and reuse these masked arrays for the rest of the analysis?
There was a problem hiding this comment.
For many of the functions in this file I get the feeling that separating them out into functions might be a bit too much. e.g. the absolute_error and relative_error functions are extremely small and are pretty basic operations in numpy.
For now we can leave it in, but I would suggest that we clean this up a bit in the future.
There was a problem hiding this comment.
I love the simplicity of and ease to run this notebook!
| ) | ||
|
|
||
| return electron_acceptors_out, utilization_factor_out | ||
| # def _check_instant_reaction_acceptor_input(electron_acceptors, utilization_factor): |
There was a problem hiding this comment.
I agree with sonarcloud on this, it's good practice not to have commented out code inside your source. Either remove it, or keep it in. You can always create an issue with a reference to this line in a previous version and some context on why you would like to remember this.



Finalize mibitrans examples for documentation
Fixes #153
Fixes #147
Fixes #133