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

Edit Domain: add custom format #6848

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

ajdapretnar
Copy link
Contributor

Issue

Fixes #6841.

Description of changes

Add custom format option to Edit Domain.

Includes
  • Code changes
  • Tests
  • Documentation

Copy link

codecov bot commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 47.36842% with 10 lines in your changes missing coverage. Please review.

Project coverage is 88.25%. Comparing base (910d625) to head (ffb1981).

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 Show resolved Hide resolved
Orange/widgets/data/oweditdomain.py Outdated Show resolved Hide resolved
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":
Copy link
Contributor

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).

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()
Copy link
Contributor

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.)

Copy link
Contributor

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()

Orange/widgets/data/oweditdomain.py Outdated Show resolved Hide resolved
Orange/widgets/data/oweditdomain.py Outdated Show resolved Hide resolved
Orange/widgets/data/oweditdomain.py Outdated Show resolved Hide resolved
@ajdapretnar
Copy link
Contributor Author

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:

  • Add a working test/tests.
  • Add report.
  • Update documentation.
  • Remove some (now unnecessary) formats.
  • Add a translation.

Copy link
Contributor

@janezd janezd left a 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))

Copy link
Contributor

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.

Copy link
Contributor Author

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")
Copy link
Contributor

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"))
Copy link
Contributor

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.

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

Successfully merging this pull request may close these issues.

Edit Domain interpret as time: add more dd-mm options
2 participants