[PULL REQUEST] VPM-B

[Dirk Hohndel]

On Tue, Jun 30, 2015 at 12:02:08AM +0200, Jan Darowski wrote:
> Hi, here is the request for the basic vpm algorithm. Right now it
> doesn't include Boyle's law compensation so the deco plans it
> generates can be too short in some situations.

Jan, thanks for sending this. I have asked Robert to look at the code from
the algorithmic side, but let me give you some general feedback in
addition to the comments that I made on github:

Commit structure:
- there is a pattern where one commit adds something and the next commit
  removes it. I understand that while working on code this is something
  that happens relatively frequently. And it is something that for a
  number of reasons way too often makes it into our master branch. But
  especially when creating a pull request like this as part of a GSoC
  project with the goal to teach you how to do well in open source it
  would be really good to avoid those. Basically go back and never add the
  parameter to begin with.
- granularity: you start out with nice small, self contained commits but
  it quickly turns into bigger changes that mix clearly unrelated things
  into the same commit. I'd like to see these redone and broken down into
  much more easily understandable segments

Commit messages:
- the title should be in present tense. It describes what the commit does
  (so "Add" instead of "Added"). For series like this it's nice to have a
  consistent prefix (I often try to do that, but it's kinda optional)
- the commit message should describe what the commit does and why
- it is not unusual at all to have a commit message that is longer than
  the patch - your commit messages in general are severely lacking;
  there's either nothing at all or a few extremely high level bullets;
  even the titles tend to be generic instead of specific
- when you refactor the commits into smaller pieces as I suggested above,
  it becomes much easier to just write a few sentences about the ONE THING
  that the commit does; and subsequently it becomes much easier for the
  person reading the commit (as review, or when chasing a bug) to wrap
  their mind around things.

Coding style:
- overall not terrible, but I would have preferred to see all the
  whitespace mess cleaned up and not committed and then later fixed
- since I'm suggesting that you refactor your commits anyway it would be
  nice to have that addressed.


This is open source so there are many possible paths forward. My preferred
path would be that you spend a few hours to learn the magic of git rebase
and refactor the commits along these ideas. While that is sometimes
frustrating work I think it would be a good learning experience and would
strengthen the work that you are submitting as part of GSoC.

/D


> The following changes since commit f763da66b3db17347954272b9f856df6f8b9888d:
> 
>   Implement planner option to switch only at required stops
> (2015-06-26 06:14:28 -0700)
> 
> are available in the git repository at:
> 
>   https://github.com/Slagvi/subsurface.git beta-vpm
> 
> for you to fetch changes up to 2ce3f0289d446a1b32a932128f1640dc0bc11291:
> 
>   Cleaned up VPM-B work, improved coding style, removed redundant
> conditions. (2015-06-26 20:47:13 +0200)
> 
> ----------------------------------------------------------------
> Jan Darowski (11):
>       Added basic VPM-B deco algorithm settings.
>       Added crushing pressure state and is_vpmb parameter.
>       Added basic crushing pressure calculation,     without
> calculating current radius in the impermeable part.
>       Added simple radius calculation for crushing pressure.     Using
> bin search with const number of iterations.     Refactored crushing
> pressure a little bit.
>       Added simple nuclear regeneration.     All the parameters for
> the deco should be ready.     Probably units adjustments needed.
>       Fixed vpmb rs value in nuclear regeneration.
>       VPM-B improvements:       Scaled constants       Added initial
> supersaturation calculation       Fixed minor calculation bugs
>       VPM-B critical volume algorithm implemented
>       VPM-B CVA fixed.
>       Added ui option to choose deco algorithm.
>       Cleaned up VPM-B work,     improved coding style,     removed
> redundant conditions.
> 
>  deco.c                         | 168 +++++++++++++++++++++++
>  dive.h                         |   4 +
>  planner.c                      | 298 ++++++++++++++++++++++++-----------------
>  pref.h                         |   8 +-
>  qt-models/diveplannermodel.cpp |   6 +-
>  qt-models/diveplannermodel.h   |   2 +-
>  qt-ui/diveplanner.cpp          |  20 ++-
>  qt-ui/diveplanner.h            |   2 +
>  qt-ui/plannerSettings.ui       |  23 +++-
>  subsurfacestartup.c            |   4 +-
>  10 files changed, 398 insertions(+), 137 deletions(-)
> _______________________________________________
> subsurface mailing list
> subsurface at subsurface-divelog.org
> http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface