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

Update objects when path changes for MultiFileSelector and FileSelector #546

Closed
wants to merge 9 commits into from

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Oct 8, 2021

Fixes #545 (only addresses the update path issue, not the default value)

import param

class A(param.Parameterized):
    fs = param.FileSelector(path='/usr/share/*')
    mfs = param.MultiFileSelector(path='/usr/share/*')

a = A()

print(a.fs is None)  # True
paths = a.param.fs.objects.copy()
a.param.fs.path = '/usr/bin/*'
print(a.param.fs.objects == paths)  # False

print(a.mfs is None)  # True
paths = a.param.mfs.objects.copy()
a.param.mfs.path = '/usr/bin/*'
print(a.param.mfs.objects == paths)  # False

param/__init__.py Outdated Show resolved Hide resolved
if self.default in self.objects:
return
self.default = self.objects[0] if self.objects else None
self.default = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path support looks great now, thanks! I'm confused about the default handling, though. #545 says that the default is None here and in Multifileselector (below), which appears to be the case after these diffs, but it also says that the default value handling isn't changed, yet here and below the diffs are explicitly changing the default value handling. What am I missing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before these diffs default is None indeed, despite the fact that it seems to be set to the first path in update(). This is because the base class constructor (Selector) is called with default=default (note that it's not default=self.default) and empty_default=True which means that the autodefault mechanism isn't used.

I was also slightly confused by how default should behave. When the objects attribute of a Selector is changed the default value isn't updated:

import param

class P(param.Parameterized):
    s = param.Selector(objects=list('abc'))

p = P()

print(p.param.s.default)  # 'a'

p.param.s.objects = list('def')

print(p.param.s.default)  # 'a'

While FileSelector was not setting default when initialized, it was however possible to update it by calling update(), as long as objects.

class A(param.Parameterized):
    fs = param.FileSelector(path='/usr/share/*')

a = A()

print(a.param.fs.default)  # None

a.param.fs.path = '/usr/bin/*'
a.param.fs.update()

print(a.param.fs.default)  # '/usr/bin/2to3-'

With this PR, default is None if not declared by the user. If declared and update() is called, it is set to None if the previous default path is not one of the new available paths listed in objects:

class A(param.Parameterized):
    fs1 = param.FileSelector(path='/usr/share/*')
    fs2 = param.FileSelector(path='/usr/share/*', default='/usr/share/CSI')

a = A()

print(a.param.fs1.default, a.param.fs2.default)  # None, '/usr/share/CSI'

a.param.fs1.path = '/usr/share/*'
a.param.fs2.path = '/usr/share/*'
a.param.fs1.update()
a.param.fs2.update()

print(a.param.fs1.default, a.param.fs2.default)  # None, '/usr/share/CSI'

a.param.fs1.path = '/usr/bin/*'
a.param.fs2.path = '/usr/bin/*'
a.param.fs1.update()
a.param.fs2.update()

print(a.param.fs1.default, a.param.fs2.default)  # None, None

But this is quite an arbitrary decision, default could also be not reset and just stick to whatever the first value is declared when the Parameter is initialized (as done by Selector). Let me know what you think :)

(update() needs a docstring, so once we settle on a behavior it can be explicitly described)

@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@090d1c4). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #546   +/-   ##
=========================================
  Coverage          ?   82.13%           
=========================================
  Files             ?        4           
  Lines             ?     2950           
  Branches          ?        0           
=========================================
  Hits              ?     2423           
  Misses            ?      527           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 090d1c4...75dfc79. Read the comment docs.

@maximlt
Copy link
Member Author

maximlt commented Oct 12, 2021

@jbednar I've:

  • reverted to the original behavior for default, which is (for both FileSelector and MultiFileSelector) to have default=None if not specified by the user, and that can then be updated by calling '.update()', to 'self.objects[0]' for FileSelector and 'self.objects' for MultiFileSelector
  • Removed _on_set (a method called when an attribute is set on a Parameter) since it was just not working in this case (it was calling .update(), itself using self.path which still pointed to the old attribute value)
  • Updated the user guide to describe these behaviors

@philippjfr
Copy link
Member

This looks good. @maximlt can you advise if this PR still should be merged and @jbednar can you please either approve and merge or suggest what other fixes are needed?

@droumis droumis added this to the v1.12.x milestone Jan 16, 2023
Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an appropriate change to me. We might want to revisit after #605 to make the default always be the first value or all values (for multi) unless the user has explicitly provided a default of None (currently not possible to detect).

"\n",
"A `param.MultiFileSelector` is the analog of ListSelector but for files, i.e., again supporting a path glob but allowing the user to select a list of filenames rather than a single filename. The default value in this case is _all_ of the matched files, not just the first one."
"A `param.MultiFileSelector` is the analog of ListSelector but for files, i.e., again supporting a path glob but allowing the user to select a list of filenames rather than a single filename. The default value in this case is _all_ of the matched files, not just the first one. The default value is `None` unless specified.\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to say two different values for the default, which presumably can't be true?

@maximlt maximlt modified the milestones: v1.12.x, 2.0 Apr 4, 2023
@jbednar
Copy link
Member

jbednar commented May 12, 2023

I think is is ready for revision by @maximlt now that #605 was merged. I think it makes more sense for the default always be the first value or all values (for multi) unless the user has explicitly provided a default of None; None is not a very useful default. In any case, the PR title needs updating to reflect the actual changes in the diffs.

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