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

Fix writing dates/times to dta #145

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@diogocp
Copy link
Contributor

diogocp commented Dec 4, 2015

Writing dates and datetimes to dta is currently broken (#139), since haven does not convert them to the Stata epoch (1960-01-01). This pull request fixes that.

Instead of adding more conditionals to write_sav, I opted to create separate write_dta and write_sav methods. I feel that this makes it easier for me and others who are only familiar with one of the formats to contribute. I also expect the two functions to diverge further when #143 is fixed. Perhaps it would make sense at some point in the future to have separate classes for each of the formats inheriting from DfWriter.

Closes #139
Closes #140
Closes #144

diogocp added some commits Dec 3, 2015

DfWriter: separate methods for writing sav and dta
Using write_sav to write both sav and dta makes it hard to implement
more than the basic features of each format. This commit adds a new
write_dta method that is essentially a duplicate of write_sav, but will
be expanded in the future with dta-specific features.
Saving non-integer labelleds to dta is unsupported
Stata only supports labelled integers. This commit makes write_dta emit
a warning when trying to write non-integer labelleds. The data are
written, but the labels are discarded.

Closes #144
defineVariable(as<NumericVector>(col), name, "%td");
} else if (col.inherits("POSIXct")) {
defineVariable(as<NumericVector>(col), name, "%tc");
} else switch(TYPEOF(col)) {

This comment has been minimized.

@hadley

hadley May 30, 2016

Member

Can you use an explicit {} block here please?

double val = REAL(col)[i];
if (ISNAN(val)) {
readstat_insert_missing_value(writer_, var);
}

This comment has been minimized.

@hadley

hadley May 30, 2016

Member

else needs to go on same line as }

@hadley

This comment has been minimized.

Copy link
Member

hadley commented May 30, 2016

My main concern with this approach is that it does lead to a lot of duplicated code, but I think you're right that this is inevitable as the sav and dta writes will continues to diverge overtime.

Could you please complete the split by removing the type_ field from Writer?

@hadley

This comment has been minimized.

Copy link
Member

hadley commented May 30, 2016

Actually let me do a little refactoring first

readstat_insert_double_value(writer_, var, val);
}
} else if (col.inherits("POSIXct")) {
double val = REAL(col)[i];

This comment has been minimized.

@hadley

hadley May 30, 2016

Member

This needs a bit more thought because POSIXcts can be either doubles or integers

@hadley

This comment has been minimized.

Copy link
Member

hadley commented May 30, 2016

Done in 944466b

@hadley hadley closed this May 30, 2016

@lock lock bot locked and limited conversation to collaborators Jun 26, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.