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

The method set_units.mixed_units doesn't preserve a vector's ordering #271

Closed
kgluborg opened this issue Mar 10, 2021 · 6 comments · Fixed by #272
Closed

The method set_units.mixed_units doesn't preserve a vector's ordering #271

kgluborg opened this issue Mar 10, 2021 · 6 comments · Fixed by #272
Labels

Comments

@kgluborg
Copy link

kgluborg commented Mar 10, 2021

I deal with a lot of identical data coming from different sources, each using different units. The mixed_units class and function are very convenient to harmonize everything but since I upgraded to units v0.7 I have found that applying set_units() to a mixed_units vector now mess up the ordering of the initial vector.

library(units)
#> Warning: package 'units' was built under R version 4.0.4
#> udunits database from H:/HOME/Personal/R/win-library/4.0/units/share/udunits/udunits2.xml
a <- 3001:3010
b <- rep(c('m', 'yard'), 5)
c <- mixed_units(a,b)

c
#> Mixed units: m (5), yard (5)
#> 3001 [m], 3002 [yard], 3003 [m], 3004 [yard], 3005 [m], 3006 [yard], 3007 [m], 3008 [yard], 3009 [m], 3010 [yard]

set_units(c, 'km', mode = "standard")
#> Mixed units: km (10)
#> 3.001 [km], 3.003 [km], 3.005 [km], 3.007 [km], 3.009 [km], 2.745029 [km], 2.746858 [km], 2.748686 [km], 2.750515 [km], 2.752344 [km]

The order of values is not preserved, the old 'm' are at the head of vector and the old 'yard' at the tail

The quick fix I had to use to preserve the sequence:

library(purrr)
set_units(map2_dbl(c,'km', function(x,y) set_units(x,y,mode = 'standard')), 'km', mode = 'standard')
#> Units: [km]
#> [1] 3.001000 2.745029 3.003000 2.746858 3.005000 2.748686 3.007000 2.750515
#> [9] 3.009000 2.752344

@Enchufa2
Copy link
Member

Thanks for the report.

@edzer This regression is worth a patch release.

@edzer
Copy link
Member

edzer commented Mar 11, 2021

submitted 0.7-1...

@edzer
Copy link
Member

edzer commented Mar 11, 2021

Ah, we overlooked these:

Dear maintainer,
 
package units_0.7-1.tar.gz has been auto-processed.
The auto-check found additional issues for the *last* version released on CRAN:
  rcnst <https://github.com/kalibera/cran-checks/tree/master/rcnst/results/units>
  valgrind <https://www.stats.ox.ac.uk/pub/bdr/memtests/valgrind/units>
CRAN incoming checks do not test for these additional issues and you will need an appropriately instrumented build of R to reproduce these.
Hence please reply-all and explain: Have these been fixed?
 
Log dir: <https://win-builder.r-project.org/incoming_pretest/units_0.7-1_20210311_102122/>
The files will be removed after roughly 7 days.
Installation time in seconds: 65
Check time in seconds: 156 
R Under development (unstable) (2021-03-10 r80084)
 
Pretests results:
Windows: <https://win-builder.r-project.org/incoming_pretest/units_0.7-1_20210311_102122/Windows/00check.log>
Status: OK
Debian: <https://win-builder.r-project.org/incoming_pretest/units_0.7-1_20210311_102122/Debian/00check.log>
Status: OK

@edzer
Copy link
Member

edzer commented Mar 11, 2021

I can't easily find the rcnst problem in units (happens in dplyr?), but valgrind seems to indicate sth with mixed units.

@Enchufa2
Copy link
Member

The rcnst issue is in dplyr. That check was done with dplyr 1.0.4. The new version 1.0.5 should have solved it, but CRAN has to re-run the check.

The valgrind issue is new. Quite strange, because nothing has changed in our C++ side. Let me investigate.

@Enchufa2
Copy link
Member

Ok, it's the same problem we had at exit due to the particular memory management model udunits2 has, which requires us to force weak finalizers before freeing the sys object. Now we have that same problem at startup, because now we allow changing the units system via load_units_xml(). So I've centralized the call to the garbage collector in 3e451b3, and there should be no valgrind issues now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants