-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Edit Domain: add custom format #6848
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6848 +/- ##
==========================================
- Coverage 88.26% 88.25% -0.02%
==========================================
Files 326 326
Lines 71146 71165 +19
==========================================
+ Hits 62796 62805 +9
- Misses 8350 8360 +10 |
Orange/widgets/data/oweditdomain.py
Outdated
pattern2 = re.compile(time_pat) | ||
self.have_date = int(bool(pattern1.search(self.custom_text))) | ||
self.have_time = int(bool(pattern2.search(self.custom_text))) | ||
if self.format_cb.currentText() != "Custom format": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define a constant CUSTOM_FORMAT = "Custom format"
and use it when adding the item to combo and here. Otherwise, one can change the combo label and forget to change it here (and elsewhere, as necessary).
Orange/widgets/data/oweditdomain.py
Outdated
self.have_time = int(bool(pattern2.search(self.custom_text))) | ||
if self.format_cb.currentText() != "Custom format": | ||
self.format_cb.setCurrentIndex(-1) | ||
self.format_cb.emit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proper call is self.format_cb.currentIndexChanged.emit()
. But don't do this. It's better to call self.variable_changed
directly, so the flow is more transparent. Also, self.variable_changed
perhaps does something that shouldn't be done if the change is triggered by the user editing this line edit, in which case we'd split self.variable_changed
in two functions and only call the second part here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, I think that you can't/shouldn't emit this signal. setCurrentIndex
will already emit this signal (see https://doc.qt.io/qt-6/qcombobox.html#currentIndex-prop). That is, the proper code would be
self.variable_changed()
but this will call self.variable_changed()
twice (once because of the change in combo and once because of the change here), so you should probably write
if self.format_cb.currentText() != CUSTOM_FORMAT:
self.format_cb.setCurrentIndex(self.format_cb.count() - 1)
else:
self.variable_changed()
I think this should work now. The lineEdit should handle also weird/random characters. I would test, but I cannot get the test to work. I've tried everything I could think of. See test for reference. TODO:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a test with a format the includes hours, minutes or seconds (have_time
), and perhaps some other formats, too.
model = self.widget.domain_view.model() | ||
index = self.widget.variables_view.model().index | ||
self.widget.variables_view.setCurrentIndex(index(0)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your original variable is a string. You must change its type to time variable, like this:
editor = self.widget.findChild(VariableEditor)
tc = editor.layout().currentWidget().findChild(QComboBox,
name="type-combo")
tc.setCurrentIndex(3) # time variable
tc.activated.emit(3)
When you saw an "unchanged" date, it was because your variable remained a string, and changes that you made in the edit dialog were ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the variable didn't change (it stayed StringVariable). I said I tried everything, we even had a look at the code with Lan, but couldn't figure out how to write a proper call.
output = self.get_output(self.widget.Outputs.data) | ||
print(output.domain.metas) | ||
self.assertEqual(str(table[0].metas[0]), "2024-001") | ||
self.assertEqual(str(output[0].metas[0]), "2024-01-01") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
output[0].metas[0]
is a numpy.float64
. (Don't ask why; output[0]
is Instance
, but .metas
returns a numpy array. This ancient part of Orange is a mess and it's better not to use it.) A proper test would be
var = output.domain.metas[0]
self.assertIsInstance(var, TimeVariable)
self.assertEqual(var.str_val(output[0].metas[0]), "2024-01-01")
The last line replaces what you tested. The assertion before that tests that variable type is indeed change; this test would fail with your previous code.
A better alternative to the last assertion is
self.assertEqual(var.str_val(output.metas[0, 0]), "2024-01-01")
because it avoids Instance
. Or, even better.
self.assertEqual(
output.metas[0, 0],
datetime.strptime("2024-001", "%Y-%j").replace(tzinfo=timezone.utc).timestamp())
because it avoids turning the date into string and tests that the number representing the date matches the date from the test. This makes it easier to test a few other formats, including those that also have time (e.g. MH:MM), not just dates.
@@ -3080,4 +3132,5 @@ def column_str_repr_string( | |||
|
|||
|
|||
if __name__ == "__main__": # pragma: no cover | |||
WidgetPreview(OWEditDomain).run(Orange.data.Table("iris")) | |||
WidgetPreview(OWEditDomain).run(Orange.data.Table( | |||
"/Users/ajda/Desktop/test.csv")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to remove this! I suggest you remove it now, and when you're making commits, add with add -p
and skip this chunk.
Issue
Fixes #6841.
Description of changes
Add custom format option to Edit Domain.
Includes