Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Movement issue39: adjust code to incorporate new flux model objects #58

Merged

Conversation

KathrinTessella
Copy link
Contributor

@goldingn : This change is not completed done yet, but still would like you to have a look as this is a bigger change.
Outstanding issues are

  1. I need to update the tests in the 'test_main_methods.R' which I uncomment for now and which require a partly rewriting.
  2. The continuum.flux() method has if/else statements for the flux model used. To minimize the changes required, I added into each flux_model definition a further 'model_string' as parameter (see gravity() definition ect). A potential better way could be to use the class inheritance in R and assign to each flux model a relevant class, e.g. gravity() -> gravity class etc and then all these flux model classes inherit from the superclass 'flux'. With this method I can make the switch on the class type and NOT a string parameter. But haven't looked much into inheritance of R - @goldingn What do you think about this idea ?

…ls. This change is not complete as the continuum.flux() function requires further adjustment based on the flux.model selected
…jects. The code is now running but not yet sure if the results are equivalent. Require further testing
…ng the returned optimresult be not consistent with the results before the refactoring
… generic 'object' which could fail the build again
@goldingn
Copy link
Member

Hmm, I suspect a better approach (since it will also make the code more readable and therefore easier for me to check the code matches up with the docs) would be to split up the function continuum.flux into separate functions for each of these flux objects, then delete continuum.flux altogether.

It would also be handy to record the name of flux approach in the flux object so that we can print it in summaries of the flux and movement_model objects. I.e.:

fl <- intervening.opportunities()
fl$name
[1] 'intervening opportunities'

@codecov-io
Copy link

Current coverage is 70.90%

Merging #58 into master will increase coverage by +2.45% as of 2e26bfa

@@            master    #58   diff @@
=====================================
  Files            1      1       
  Stmts          485    519    +34
  Branches         0      0       
  Methods          0      0       
=====================================
+ Hit            332    368    +36
  Partial          0      0       
+ Missed         153    151     -2

Review entire Coverage Diff as of 2e26bfa

Powered by Codecov. Updated on successful CI builds.

@KathrinTessella
Copy link
Contributor Author

@goldingn :once the checks for travis-ci are completed for my last commit, I would be happy will all changes addressing this pull request. In my last commits I addressed all the outstanding issues I mentioned above and I made the change to the continuum.flux function as you suggested.
I make some wording changes for the various continuum flux models, but hat might need further work which could be done together with issue #52.

Please have a look and let me know if you are happy for me to merge this (bigger) change. Thanks.

@goldingn
Copy link
Member

Looks great to me!

goldingn added a commit that referenced this pull request Oct 22, 2015
…-models

Movement issue39: adjust code to incorporate new flux model objects
@goldingn goldingn merged commit 9ad3388 into SEEG-Oxford:master Oct 22, 2015
@KathrinTessella KathrinTessella deleted the movement-issue39-use-flux-models branch October 22, 2015 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants