ncskth / bifrost

GNU Lesser General Public License v3.0
4 stars 1 forks source link

sorry, massive changes for a first pr :( #4

Closed chanokin closed 3 years ago

chanokin commented 3 years ago
* The `export_cell_params` in `export/population.py` is a little hard for me to understand. Can we in any way wrap that in the context class? It could also make sense to retain here. I may just not be understanding it.

Not really sure what is the best way to do this. The idea here was to generate a dictionary from the 'checkpoint' file and what the parameter context provides. Again, not quite sure where to go from here.

* Of minor comments, I have a slightly hard time reading some of your variable names. I would prefer `layer_parameters` instead of `lparams`, for instance.

Yup, I'll do a pass to rename weird shit

* I also appreciate your emphasis on spacing in the final output, but I was never under the impression that the output code should be humanly readable. Should that change? Would it make sense to generate "pretty" code?

My issue with this is that if we need to inspect the code to figure stuff out, the file is super annoying to read.

As for 'compacting' the variables into dictionaries, when I open the files in an IDE, my computer turns into a turtle due to syntax inspection. Plus, I think it's less prone to errors.

* Finally, I think we should have some more tests. I realize I'm also to blame, but I would maybe suggest we get some coverage statistics soon, just so we can know how good/bad we are :-)

Agreed, I'll start on more tests now that I've managed to run a 'full' script.

Jegp commented 3 years ago

Cool. I'm buying your arguments on spacing, and I think we agree on the naming stuff. If you'd like to, please to ahead with the smaller changes. Otherwise, I'd suggest we go ahead and merge it. Would you prefer that I do the merge? :)

chanokin commented 3 years ago

I'll merge after adding some more tests