erwanp / qtplaskin

A graphical tool to explore ZdPlaskin Results
GNU Lesser General Public License v3.0
7 stars 5 forks source link

Fix: Replace floor division with regular division #1

Closed githeap closed 5 years ago

githeap commented 5 years ago

Sensitivity analysis is broken. Not all reactions are shown on Production and Losses graphs (even when "Show all" option is selected). For some species it works OK, but generally some reactions are missing.

Please, consider the following: r, fpos, spos, fneg, sneg are all numpy arrays of type float64. Floor division ' // ' truncates fpos and fneg that is why 'select_rates' function gives wrong results. Original QtPlaskin uses regular division ' / ' and I can't see a reason why we should change it. Let me know if you need more data for test.

--- Edit @erwanp

erwanp commented 5 years ago

Hello @githeap.

The floor division was introduced in commit 41aacbe as part of the Python 3 translation (likely performed by an automatic tool). It may indeed be erroneous. This is a very important finding!

Before merging I'd like to have a Test that would fail before and pass now. This way we'd ensure such issues do not arise anymore.

I'm thinking about adapting one of the four ZdPlaskin Example. None of them is currently configured to use QtPlaskin and that would require a minor modification. Do you have any other example in mind?

githeap commented 5 years ago

Hello @erwanp . I have added small fake dataset for testing. I guess it would be much easier to debug using this dataset than ZdPlaskin Examples. I also had to extract rate selection logic from update_source_graph method, because otherwise I needed to simulate GUI events. I am not an expert in PyQt5 and generally it is a bad idea to couple logic with representation(GUI). I extracted selection logic to select_rates function and renamed old select_rates to filter_rates. Rename old stuff is not great , not terrible, I hope. (I found no other usages of it.) I wrote tests, which you can run using pytest module. To make them fail edit select_rates function. Unfortunately, because of refactoring you can't run them with old code. Also check that test imports select_rates from NEW instance of qtplaskin.main.

erwanp commented 5 years ago

I could get the test running, well done! Merging.

erwanp commented 5 years ago

@githeap : I released a new version 1.1.1 (#4) which includes your fixes. Starting from this version tests are performed automatically on Travis CI so they do not break ever again.

Travis uses pytest so any new test that you may add in the future will be run automatically too! This will make the Pull Request process much faster.