davidclarance / rabm

The `rabm` package provides an interface to the Africa Bird Atlas data.
https://davidclarance.github.io/rabm/
3 stars 1 forks source link

Correct for empty request #15

Closed Rafnuss closed 4 years ago

Rafnuss commented 4 years ago

Also implemented solution proposed in #9 in 50e6e3a and second solution discussed in #17

Rafnuss commented 4 years ago

If you like the solution, it should also me implemented for the other extract function.

davidclarance commented 4 years ago

Thanks Raf! This is awesome work. A few comments:

  1. 9 solution looks great.

  2. The solution for extract_species looks great but we might need to clean and test a bit. It seems to be returning empty results for any request I make. Additionally, it is still failing for multiple pentads. I think we'll need to redo the function in its entirety. We should move the pull_for_species function out and make it an independent function where we can do more work on it.

So the steps to clean up this function will be:

  1. Create new function called pull_for_single_species. This function should be able to pull data for multiple pentads, countries etc for one single species. This function will need to internally loop over multiple pentads/countries etc (whatever the defined unit is).

  2. Call pull_for_single_species in extract_species, after checking for arguments and counts etc.

Happy for you to take a shot at it, otherwise I'll look at it in the night or early tomorrow morning (or later if you prefer).

Thanks for contributing!

Rafnuss commented 4 years ago

I can have a look today. Is there a reason why not looping through species AND locations (region_ids) in the main function (extract_species) and then only allows a single single specie and location in pull_for_single_species. Would you be ok for me to implement ArgumentCheck?

davidclarance commented 4 years ago

@Rafnuss yeah I think that would be much more elegant. One species and location in the function and the loop through species and locations in extract_species. In that case we should then name the function as pull_single_species_location just to be more consistent. Great idea, thanks!

Yeah and implementing argument check would be good.

Rafnuss commented 4 years ago

Here is my proposition:

Rafnuss commented 4 years ago

A first try at testthat is proposed in ea339d1 adressing #10, #11 and #12. Happy to get feedback on this

davidclarance commented 4 years ago

@Rafnuss I've changed the base branch for the PR to dev and merging it. This is to allow the changes you've made to be tested. Once I've finalized everything in the dev branch, we'll merge to master.