lgerhardt45 / Jointify

Digitalizing ROM analysis for the MRI hospital, Munich, as part of the Tech Challenge
5 stars 0 forks source link

Improve analysis speed and show only max and min frame #59

Closed BergmuellerNiklas closed 4 years ago

BergmuellerNiklas commented 4 years ago

Sorry for this rather big PR, but the logic is pretty dependant on each other. So this was inevitable unfortunately.

What works now:

What does not work:

lgerhardt45 commented 4 years ago

I'll have a look and review this PR later but can see a rather big problem in the title already. You say you do two things:

  1. Improve analysis speed
  2. Show only max and min frame

If we now think to revoke the show only max and min frame functionality, we could just revert to a commit previous to the one where you add it with a PR. Changing this functionality now required you (and only you, because only you implemented it and know directly where it is) to go back in, make a new PR etc. what makes us lose time.

If possible, I would actually like to ask you to remove this functionality, create a new branch (even from this branch if you want) and create a new PR. Again, one PR per functionality. If that's not possible we will deal with it :-).

BergmuellerNiklas commented 4 years ago

Well done, thanks a lot! I added suggestions to most of my comments so re-request feedback when implemented.

I would not be able to say where exactly you applied changes to only send the min and max values without more thorough inspection. IMO, this should

  1. be done in a separate PR (which would help me understand it better) and
  2. should rather be done on the frontend.

Maybe we consider showing them at some other point and then I, @annalena94 or @lennoxjay are more flexible in adjusting it rather than going down into your code. The job of the ML model is to analyse all frames and pass them back up (or as we suggested only pass up the CGPoints of the joints and the degree). Holding the images back in the backend is IMO not the way to go.

I already discussed this with @lennoxjay this morning. My main rationale was practicability. I am totally aware that usually it is better to create small iterative PRs, so that we would be able to reset them if problems occur. However, I feel like we are running out of time, since the complete app has to be ready until Tuesday (assuming that we want to show a screen recording on the dry run to get feedback). Since I was touching the same code anyway I felt like it would make more sense to include one PR that you guys can review so that we safe precious time (otherwise I would have needed to wait until the first PR is merged etc). And with all that video and presentation stuff I will be doing on the weekend, I would not have time to finish this otherwise.

If you don't think this is a good way to handle the images, I am more than happy to discuss refactoring it. But for now, let's use it and get this thing (mostly) done by Tuesday.

If we decide to change this logic, I take complete responsibility for the refactoring obviously.

lgerhardt45 commented 4 years ago

Waiting for the Build to run through, will merge tomorrow morning 👍