[ENH] change _MetaObjectMixin._check_objects default for attr_name to auto-detect

[SimonBlanke]

Reference Issues/PRs

Closes #159

What does this implement/fix? Explain your changes.

Sets attr_name to None and auto detect from "named_object_parameters" of attr_name remains None

Does your contribution introduce a new dependency? If yes, which one?

What should a reviewer concentrate their feedback on?

Any other comments?

PR checklist

For all contributions

• I've reviewed the project documentation on contributing
• I've added myself to the list of contributors.
• The PR title starts with either [ENH], [CI/CD], [MNT], [DOC], or [BUG] indicating whether
  the PR topic is related to enhancement, CI/CD, maintenance, documentation, or a bug.

For code contributions

• Unit tests have been added covering code functionality
• Appropriate docstrings have been added (see documentation standards)
• New public functionality has been added to the API Reference

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.00%. Comparing base (306958d) to head (1a0c96d).
⚠️ Report is 180 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #466      +/-   ##
==========================================
- Coverage   85.07%   84.00%   -1.08%     
==========================================
  Files          45       52       +7     
  Lines        3015     3913     +898     
==========================================
+ Hits         2565     3287     +722     
- Misses        450      626     +176     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[SimonBlanke]

added a test 'test_check_objects_attr_name_explicit'. The asserts are not that useful, but it at least covers some additional code if attr_name is not None

[SimonBlanke]

added another test 'test_check_objects_attr_name_custom_tag'. It sets a custom tag via set_tags.

I just realized, that I used a private method _check_objects for the tests. Normally, that should be a no-go, but it seems the method is not used anywhere else in the package.

[SimonBlanke]

@fkiraly I improved the test quality by triggering an error in each test and catching the error type.

[fkiraly]

I think the coupling to self should happen at least one layer higher, i.e., not in the checker function, but where it is being called.

Otherwise we have intransparent coupling in a function that looks like a pure function but actually is not.

[SimonBlanke]

I think the coupling to self should happen at least one layer higher, i.e., not in the checker function, but where it is being called.

Otherwise we have intransparent coupling in a function that looks like a pure function but actually is not.

I understand we have some hidden coupling going on in _check_objects. The method signature does not reveal how it works. The tags are surprisingly read without the user knowing. From your explanation I infer the problem is, that some high level orchestration stuff (object stats, method calls) is mixed with lower level logic. It would help if we separate this.

I'll think about how to solve this.

[fkiraly]

that some high level orchestration stuff (object stats, method calls) is mixed with lower level logic. It would help if we separate this.

Yes, exactly - are there any other tags that get read in there?

[SimonBlanke]

are there any other tags that get read in there?

No, I only see attr_name = self.get_tag("named_object_parameters") here. For the decoupling I would like to separate the code into the following methods:

def _check_objects(self, objs, attr_name=None, ...):
    """Orchestration/high-level layer."""
    if attr_name is None:
        attr_name = self.get_tag("named_object_parameters")

    validated = self._validate_objects(objs, attr_name, ...)
    return self._coerce_to_named_object_tuples(validated, ...)

@staticmethod
def _validate_objects(objs, attr_name, cls_type=None, ...):
    # All validation logic

[SimonBlanke]

okay there is a lot going on here (or maybe this is just in my head). First I added this note, because _check_objects does not really make sense to me as a private method, right now.

_validate_objects is a staticmethod now containing all the validation logic, _check_objects handles the rest. I also added a lot of comments. It feels necessary here.

[SimonBlanke]

There is a linter error in a file I did not change:
"skbase/tests/test_base.py:389:5: B043 Do not call delattr with a constant attribute value, it is not any safer than normal property access."

[SimonBlanke]

There is a linter error in a file I did not change: "skbase/tests/test_base.py:389:5: B043 Do not call delattr with a constant attribute value, it is not any safer than normal property access."

fixed!

[fkiraly]

Looks good to me!