search

tidymodels / agua

Create and evaluate models using 'tidymodels' and 'h2o'
https://agua.tidymodels.org
Other
22 stars 2 forks source link

breaking change in upcoming tune release #55

Open simonpcouch opened 5 months ago

simonpcouch commented 5 months ago

In the tune release following 1.2.1, tune's .catch_and_log(split) argument will be renamed to .catch_and_log(split_labels), and will take the format labels(split) rather than split. agua just passes that argument once here:

https://github.com/tidymodels/agua/blob/6a742f637f3ae9ce70a7da117b8c6097355a0abf/R/tune.R#L105-L111

...and can pass it's value conditional on tune's package version.

Related to https://github.com/tidymodels/tune/pull/909.

The noted release is probably at least a couple months out, so this can be ignored for now.

qiushiyan commented 5 months ago

Sounds good. I will update in the next few days.

The noted release is probably at least a couple months out, so this can be ignored for now.

Not sure I understand this, should I just pass labels(split) or do a conditional on tune's version?

simonpcouch commented 5 months ago

The noted release is probably at least a couple months out, so this can be ignored for now.

We won't be sending tune to CRAN for at least a couple months, so this changes requested in this issue aren't needed yet. The more important move, from tidymodels' perspective, is to send your current development version out (#54 has a model for doing so) that supports the current tune version—CRAN tune and CRAN agua have been incompatible since April 18th.

should I just pass labels(split) or do a conditional on tune's version?

Conditional on tune version, either split or labels(split). Current CRAN tune needs split, the next version will need labels(split). I'm glad to put in a PR with the needed changes once you have a new release of agua out if that's appreciated!

qiushiyan commented 5 months ago

Thanks for your help @simonpcouch , new CRAN agua is released. I will hopefully be less occupied in the following months so I could make the split change once it's needed. I'll let you know if I need anything.

simonpcouch commented 5 months ago

Hey @qiushiyan, I see you've closed this issue but am not seeing the related fix on any branches. I will reopen so that it stays on our radars when we start to think about the next tune release, but feel free to close if this indeed fixed and I've missed where.

qiushiyan commented 5 months ago

I forgot to push this. It's in https://github.com/tidymodels/agua/commit/56b4e0a3bddc22e0b243bfc76fcd7abafdf39e89