NAVADMC / ADSM

A simulation of disease spread in livestock populations. Includes detection and containment simulation.
Other
10 stars 5 forks source link

Visual and User Usability Update #865

Closed ConradSelig closed 6 years ago

ConradSelig commented 6 years ago

Tickets covered in this merge:

771, #803, #815, #861, #863, #864

771: Set default ring radius to 0

803: Changed color on review disease spread description text

815: Un-squeezed production type group text

861: Added additional usability to functions panel.

863: Got graph to display when P = 0 or 1 for Negative Binomial probability type.

864: Made "Cancel" button in Import Legacy Scenario go to new scenario home screen (still performs all previous operations).

ConradSelig commented 6 years ago

Close and open due to some confusion with branches, my mistake!

BryanHurst commented 6 years ago

@ConradSelig A few notes for the future:

Please reference an issue number in commits when possible.

Please work either directly in our development branch (for trivial changes), or in a branch created from our current development branch on the ADSM repo (for batch and large changes). Having your current working branch in another repo means that I have to checkout from your page to do any testing.

BryanHurst commented 6 years ago

Please add the following comment to the submit_button method.

""" NOTE: This submit button is currently only used in the NonModelForm class which is only used in the ImportForm. ALSO NOTE: When cancel is selected, the page is sent back to the base scenario setup page. """

BryanHurst commented 6 years ago

The text on the validation screen may need some sort of logic to detect if there are indeed any warnings instead of just always assuming there are.

BryanHurst commented 6 years ago

In the ViewEditButtons, please don't use style blocks in html and instead put styles in the proper css file.

Also, please make some sort of note/comment in this file that it is currently only used in the Probability and Relational Function forms so has special logic specific to them.

BryanHurst commented 6 years ago

In scenario creator views, you renamed the string representation of ProbabilityFunction to ProbabilityDensityFunction.

If this is the desired naming, please make the change to the class and all proper places in code and string references.

Does this remove the need to pass local_name into the list_per_model function?

ConradSelig commented 6 years ago

When changing "ProbabilityFunction" to "ProbabiltyDensityFunction" (#746), my original approach was to change the actual model name. There are a total of 97 usages of "ProbabiltyFunction" in the code, and a refactor left me with database errors, because every excisting scenario we have uses probabilityfunction not probabilitydensityfunction.

Another thing I noticed was that the model name was never used. Every time the name of a function needed to be displayed it was simply parsed from the class name. After this discovery I simply adapted the code to accept the string as the name and display that. This avoids the database errors and makes changing display names really easy.

If you would like to pursue changing the model name we can do so, it's not like it's impossible to migrate the databases, I just think that the current solution makes the most sense. Maybe adding some comments in to the code to explain the difference between the string and the model name on line 27 (views.py)?

BryanHurst commented 6 years ago

On the PDF Model Name: This might be a good opportunity for you to try migrating the DB with a small change that won't break much.

If you aren't familiar with DB migrations in Django, they are pretty easy and I'd suggest looking that up.

Quick reference: /path/to/venv/python manage.py makemigrations && /path/to/venv/python manage.py migrate

ConradSelig commented 6 years ago

I went to make the migration and ran into a couple errors in fields.py (in the virtual environment) where "probabilityfunction" was not going through. I put in a couple hard conditionals to change those to "probabiltydensityfunction". This did get rid of the errors but the migration command says "no changes detected" - ADSM still gets the same database error when running it. Any idea what I should try next?

BryanHurst commented 6 years ago

I just did the rename in my code to see if I ran into any of these issues, and I was able to perform the rename and make migrations with no hiccoughs. I used the refactor function in PyCharm for renaming the model, and always make sure to never make changes to any existing migrations (so excluded the migration folders from the rename).

After that, I created the migrations, ran them, and launch the program and ran a test with no additional code changes required.

HOWEVER!! The CEngine errors out since it can't find the right table in the column.

So I'll take this part on and make changes in both Python and C. Please revert any commits that you have related to this rename (including the string only changes).

ConradSelig commented 6 years ago

OK sounds good, I think my problem likely stems from the fact I was refactoring the existing database migrations.

BryanHurst commented 6 years ago

I'm going to go ahead and merge this in so I can start making changes.