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

major rewrite of bcea() and ceac.plot() #21

Merged
merged 58 commits into from
Jul 21, 2020
Merged

Conversation

n8thangreen
Copy link
Contributor

I'm not sure why theres a merge conflict without looking in to it because I thought I forked the latest version but you'll know better than me.

There are a lot of changes, both large and small.

  • Most simply, there are style changes, such as using more line breaks and spaces
  • I've split lots of the longer functions in to smaller subfunctions, notably bcea() and ceac.plot()
  • I've tried to organise things together so version comments are in NEWS, data Rmd in data.R, options() stuff in onLoad() in zzz.R, etc

This certainly needs a good code review. Realistically, I think it could take a bit of time to do this properly.
Outstanding things to do are:

  • Finish rewriting ceplane() in the same way as ceac.plot(). At the moment its just commented out
  • Write all the tests of the bcea() subfunctions, and test for everything else too. The plots can use the vdiffr package.
  • Other things in Issues

several small refactoring (safer)

* change from . to _ separators
* c,e to cost, eff mainly because c is a built in function
* error asserts at top of bcea.default
runs but doesnt create output for vi,Ustar, ol, evi
associated tests fail
n8thangreen and others added 26 commits July 6, 2020 10:32
need to edit prepare_graph_params_ceplane()
…() apart into smaller functions similar to ceac.plot but haven't finished
realised theres loads of overlap with ceac_multi and already rewritten ceac.plot.
Also plot.bcea()
definitely doesnt run right now...
changes the dispatch to lower level funcitons
* position legent in ceac_plot_base correctly
* no legend for single line ceac_plot_ggplot
* axis labels for ggplot
Total rewrite of ceac.plot()
@giabaio giabaio merged commit ab22052 into giabaio:dev Jul 21, 2020
@giabaio
Copy link
Owner

giabaio commented Jul 21, 2020 via email

@n8thangreen
Copy link
Contributor Author

I think so. An if it needs slight changes it should be easy from now on.

@giabaio
Copy link
Owner

giabaio commented Jul 21, 2020 via email

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.

2 participants