search

crim-ca / decision-support-tool

0 stars 1 forks source link

Maxime roy/sandbox #22

Closed maximeroycrim closed 2 years ago

maximeroycrim commented 2 years ago

@JeremyFyke and @matprov ready to test the behaviour

JeremyFyke commented 2 years ago

Hi @maximeroycrim, this looks much nicer in terms of data organization, thank you! And the new .json schema in the *_v2.json files is nice and definitely good from my perspective and should be made official (if @matprov agrees). I like the "eng" and "fr" additions and also the `class DstEnum' which I think is intended to make the code less error-prone..?

A few minor additional suggestions (sorry if you are already have these on your radar):

JeremyFyke commented 2 years ago

Also, pending comments above, I'm happy to have this work merged to main, again if @matprov agrees. It looks like you did all your work in plain .py file. I think I need to switch over to using plain .py files moving forward (and get my team to do the same). Does that make sense to you..?

Also, given you've made many (good) changes to many parts of decision-support-tool.py, and also many other changes are also made in my outstanding pull request from dst-core/develop, I predict a complicated merge :). I'm happy to help with this, @matprov if this makes sense. More generally, to make development smoother in future, I wonder if decision-support-tool.py should be broken up into multiple separate files (e.g. one per Panel pipeline stage). What do you guys think?

maximeroycrim commented 2 years ago

@JeremyFyke, about plain .py for me it's easier to debug, you can discuss it with @matprov. Otherwise for the other comments, I agree with the changes you proposed. From now on, one of my colleagues (Antoine Daigneault-Demers) will take care of the next tasks

matprov commented 2 years ago

@maximeroycrim In order to get this PR merged we need the ipynb file to be committed. We can't diff a py with a ipynb and can't see the changes made to the notebook. Also, before committing, please make sure the app works as expected after the modifications you've made to the notebook.

@JeremyFyke We currently don't have a strong ipynb/py preference as it's more for convenience that we use notebook for now. But for now, let's continue in the notebook as stated before and keep the py for better PR reviews. This notebook will be easy to convert to py later on so let's focus at the right things for now.

maximeroycrim commented 2 years ago

@matprov, can you tell me how I can reproduce the bad behavior of the application ? I will make the changes in the ipynb. There was a miscommunication I believe, because I had understood that developing in the .py would be easier

matprov commented 2 years ago

@matprov, can you tell me how I can reproduce the bad behavior of the application ? I will make the changes in the ipynb. There was a miscommunication I believe, because I had understood that developing in the .py would be easier

@maximeroycrim Which bad behaviour do you refer to? We have been clear on the fact that we keep working on the ipynb file until we agree to change. So please commit the notebook version of the modifications you've made in the scope of this pull request as comparing an ipynb with a py will be cumbersome for no reason.