Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Show correct history for appending data #446

Merged
merged 17 commits into from
Nov 14, 2024
Merged

Show correct history for appending data #446

merged 17 commits into from
Nov 14, 2024

Conversation

bkloeckl
Copy link
Contributor

@bkloeckl bkloeckl commented Oct 29, 2024

Fixes #130. Changes in model.py: def append_data(self, names) to correctly show the code history

bkloeckl pushed a commit to bkloeckl/mnelab that referenced this pull request Oct 29, 2024
@cbrnr
Copy link
Owner

cbrnr commented Oct 29, 2024

I think duplicate names might still be a problem as you mentioned in #130. In my example with two identical names, if you only select one S001R04 in the append dialog, MNELAB still appends both datasets.

So I think we need a different solution. In general, there are two options:

  1. Disallow duplicate names (which you suggested)
  2. Use the underlying indices in functions that operate on multiple datasets

The first option is probably easier to implement. The second option would require a couple of changes, e.g. showing the indices in the sidebar and all dialogs that require multiple datasets. WDYT?

@bkloeckl bkloeckl changed the title [Fix issue] #130: Show correct history for appending data [Fix issue] Show correct history for appending data Oct 29, 2024
@bkloeckl bkloeckl changed the title [Fix issue] Show correct history for appending data Show correct history for appending data Oct 29, 2024
bkloeckl pushed a commit to bkloeckl/mnelab that referenced this pull request Oct 29, 2024
@cbrnr
Copy link
Owner

cbrnr commented Nov 11, 2024

I don't know why monkeypatching the decorator doesn't work, but at least this is a workaround. To improve upon that a little, we could think about adding two new parameters view=None and decorator=None to the Model initializer, which would probably make it a bit easier to use a model without a view. Or maybe we only need view=None, but we also adapt the decorator so that it can handle cases where args[0].view = None. This would probably be the nicest solution so far.

@cbrnr cbrnr force-pushed the bkloeckl branch 2 times, most recently from da72d48 to e3346d4 Compare November 14, 2024 15:23
@cbrnr cbrnr merged commit c83b3c0 into cbrnr:main Nov 14, 2024
5 checks passed
@cbrnr
Copy link
Owner

cbrnr commented Nov 14, 2024

I have parameterized the test to avoid repeating code, and I've also used the := operator to shorten some things. I think it looks really nice now. Thanks a lot for your PR @bkloeckl 🚀!

@bkloeckl bkloeckl deleted the bkloeckl branch December 6, 2024 12:08
cbrnr pushed a commit that referenced this pull request Feb 3, 2025
* Show correct history for appending data

* Show correct history for appending data

* Changelog entry to PR #446

* Apply Ruff formatting to pass style check

* fixed a bug where indexing would be off after auto_duplicate()

* simpler append implementation

* Add test_model.py with test_append_data.

* Adapted Ruff styling and Changelog

* Replaced the models dependency for a mainwindow view with a DummyView

* Added a view parameter to the model init function with a default value of None

* reverted some minor changes with how the models init is handled for tests

* Simplify and include function return value

* Tweaks

* dynamically determine dataset values during runtime for test_append_data asserts

* Parameterize test

* Fix style

* Fix for Python 3.9
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show correct history for appending data
2 participants