ropensci / RNeXML

Implementing semantically rich NeXML I/O in R
https://docs.ropensci.org/RNeXML
Other
13 stars 9 forks source link

Patch polymorphic / uncertain state type #173

Closed cboettig closed 5 years ago

cboettig commented 5 years ago

@hlapp I think this should now fix #171 / #172, but please verify. As mentioned in the commit note, this is just a really small fix to a bug in the S4 class definition of state, which defined symbol as integer type for standard states (e.g. symbol="1"), but as character type for polymorphic states (e.g. symbol="1 or 0").

Actually I believe this may actually be from what feels like an inconsistency in the NeXML schema.
In a standard state it appears to me that symbol attribute on a state is technically typed as a StandardToken, which looks to me to be defined as a restriction of xs:integer, which is I think why I typed the symbol as integer in the first place.

Meanwhile, in the NeXML schema defines symbol to be a xs:string in the case of a polymorphic state set, at least according to: http://www.nexml.org/doc/schema-1/characters/standard/#StandardUncertainStateSet

To me, this seems inconsistent, and is also the immediate cause of the bugs you identified, because R refuses to bind the integer symbol column to a character symbol column. Declaring symbol to always be a character string as I've done in the PR resolves the issue, but technically appears to be a deviation from the types declared by the schema itself.

cc'ing @rvosa for additional insight on this.

hlapp commented 5 years ago

Couple comments:

cboettig commented 5 years ago

@hlapp Thanks. I still don't follow the justification for the standard to type symbol attribute differently in polymorphic and non-polymorphic cases, that seems to be asking for trouble. But given that is what the standard says, yes, your solution is probably better. (I changed this first, assuming I had not followed the standard somehow, and only noticed later the standard really did type symbol as an integer.

Would you want to send a PR introducing

states <- states %>% dplyr::mutate_at(c("symbol"), as.character)

?

but this does resolve #171 , yes? (ERRROR Columnsymbolcan't be converted from integer to character). Just not all the issues in #172.

Yeah, I think #172 discusses this issue and also separate issues, might be easier to deal with the separate issues in separate issues.

hlapp commented 5 years ago

Yes I can do that.

hlapp commented 5 years ago

See #174.

cboettig commented 5 years ago

Closing in favor of #174