Repository navigation
Inconsistent output types between diffuse IAM functions #2837
Description
Activity
For further clarification, a clean, coherent output structure would be ideal to have a clean, clear handling of the "data flow" within ModelChain when computing IAM losses. Besides the "for the sake of coherent API".
I support changing the outputs to dictionaries. I don't see a way for having a deprecation period though, it think it'll just have to be a breaking change.
Can we do something like this:
if version<0.16: raise deprecation message return tuple else: return dictCan we do something like this:
if version<0.16: raise deprecation message return tuple else: return dictI don't see how that's possible. All versions <0.16 have already been shipped and cannot be edited, so this code would only appear in versions >=0.16 and thus not have any effect.
Ideally, the expected modification to ModelChain by @cbcrespo will already consider this change.
so, I would say either:
- @cbcrespo GSoC's modification to
ModelChainis shipped in0.17, together with the change in the IAM output format - we go for a temporary intermediate solution (as it had been suggested for the diffuse transposition output a while back), where for now we add a
return_dictbool parameter and later on this is deprecated
The first may be cleaner, but likely will likely raise warnings in the CI if this change and that of the ModelChain are done in separate branches (the ModelChain modif would be branched from the "iam output format" one, and would depend as a result on changes not found in the code base). An alternative could be to do both changes in the same branch.
The second has a more immediate implementation (
0.16) and on the side Carolina could already prep the code where this is deprecated and that, instead, would be what is merged in0.17.I would say that the versioning calendar may also play a role in deciding this. Any expected dates for
0.16and0.17?- @cbcrespo GSoC's modification to
The user is only warned when the kwarg is provided. If we add
return_dictthat defaults to current behavior, the user is only warned when the kwarg is changed to the new behavior, and I think we want the opposite (warn when old behavior is requested). But defaulting to the new behavior requires users to change their existing code to retain old behavior.So I don't see that the kwarg allows for old behavior without requiring users to modify existing code.
However,...AI suggested the following and it seems to work. I don't understand the subclassing. In this example,
f2would bemartin_ruiz_diffuse, for example.import warnings class DeprecatedTupleDict(dict): """ A dictionary that mimics a tuple for backward compatibility. Raises a DeprecationWarning when accessed like a tuple. """ def __init__(self, data, tuple_order): super().__init__(data) # Store the key sequence that maps to the old tuple order self._tuple_order = tuple_order def __iter__(self): msg = """tuple output from martin_ruiz_diffuse or schlick_diffuse is deprecated. In the future these functions will return a dict.""" # raise a warning to provide the traceback warnings.warn( msg, DeprecationWarning, stacklevel=2 ) # Yield values instead of keys to match old tuple unpacking behavior for key in self._tuple_order: yield self[key] def __getitem__(self, key): # Handle legacy integer index access (e.g., result[0]) if isinstance(key, int): msg = """tuple output from martin_ruiz_diffuse or schlick_diffuse is deprecated. In the future these functions will return a dict.""" # raise a warning to provide the traceback warnings.warn( msg, DeprecationWarning, stacklevel=2 ) dict_key = self._tuple_order[key] return super().__getitem__(dict_key) # Standard dict key lookup (e.g., result['a']) return super().__getitem__(key) def f1(a, b): return {'a': a, 'b': b} def f2(a, b): # Create the new dictionary payload data = {'a': a, 'b': b} # Define the precise order of the legacy tuple: (a, b) legacy_order = ('a', 'b') return DeprecatedTupleDict(data, legacy_order) x = f2(1, 2) print(x) x[0] x['a'] c, d = f2(1, 2)Will let @cbcrespo read and reflect on the AI suggestion.
But we could always have the deprecation warning show up independently of the kwarg value (if you have default/classic behavior you alert the output will change format in the following version; if you have
return_dictasTrueyou say that future version will have this as default and only behavior deprecating the parameter itself.The only caveat would be it being too annoying...
I think a simpler solution would be adding the
return_dictparam defaulting toFalse. When called from theArrayclass,return_dictwill beTrue, solving the inconsistency problem. Then withinmartin_ruiz_diffuseandschlick_diffuse, add:if return_dict == False: msg = ( """tuple output is deprecated. In the future this function will return a dict. To use dict output now, set return_dict=True.""" ) warnings.warn(msg, pvlibDeprecationWarning)The downside of this is that
return_dictwould eventually disappear and so this would prompt the users to change their workflows twice. @cwhanse's solution may be more hassle-free from the users' point of view.I suggest reviewing the discussion in #959 just in case it changes any minds here.
If we go ahead with returning dict instead of tuple, my 2 cents is to just make the change and not worry about it. Yes some kind of deprecation period would be nice, but these functions likely get less use than many in pvlib, it's easy for users to adapt their usage, complexity is hard to get right, and maintainer time is valuable.
Any expected dates for 0.16 and 0.17?
0.16.0 is the next release, and scheduled for mid-September. That date is mostly arbitrary though; we can make the release any time we choose.
Reacted by Adam R. Jensen and cbcrespoIf we go ahead with returning dict instead of tuple, my 2 cents is to just make the change and not worry about it.
I agree, seems easier for users than the alternatives we've discussed.
To avoid staying in a limbo for much longer, what about casting votes to quantify and track preferences?
List of options below with some technical notes as bulletpoint (let me know if I forgot something). React with the corresponding emoji(s) you prefer:
🚀 change outputs directly to
dict- deprecating change, but easy on our side and also for users to adapt when an error is raised (maybe we could make things easier for users by adding a temporary, easy-to-spot warning in the documentation of the IAM-related functions)
🎉 convert IAM output into a class that can behave both as tuple and dict, raising a warning when used as tuple (suggested by @cwhanse)
- creates no friction to users (at least for a while, since this would be later deprecated), but it's more cumbersome to review and to read
👀 add kwarg
return_dictchanging output format as a temporary solution, deprecating this in a later version- requires warning somehow users of deprecation, needs one more PR later to output dict-only and users will still have to change their code anyway, just later
👎 keep things as is
- avoids breaking change but keeps inconsistency in format with
iam.marion_diffuseand transposition for example and requires a more hard-coded / less clean implementation of the IAM losses within ModelChain
Reacted by Cliff Hansen and Rodrigo Amaro e SilvaReacted by Adam R. Jensen, Rodrigo Amaro e Silva, cbcrespo and Kevin AndersonReacted by Echedey Luis, Rodrigo Amaro e Silva and Anton DriesseHere I think majority rules, but I will express my preference to go on record that we should have quite a high bar to make breaking changes without deprecations.
Reacted by Echedey Luis
The diffuse IAM functions currently have inconsistent return types:
marion_diffusereturns a dictionary containing values for the different diffuse components.martin_ruiz_diffuseandschlick_diffusereturn two values corresponding to the sky and ground components.It would be beneficial for these functions to have a consistent API, making them easier to use interchangeably throughout pvlib (and in users' own workflows). I propose that all three functions ultimately return dictionaries with named diffuse components, matching the behavior of
marion_diffuse.Since this would be a breaking change, it would require a deprecation period, and I'm not sure what the preferred deprecation strategy is for outputs.
In the shorter term, this inconsistency is blocking work for my GSoC project (see #2750 and #2812). As a temporary compatibility measure, one possible approach would be to add an optional keyword argument (defaulting to False) that enables the new dictionary return format. This would allow downstream code to adopt the new interface before the default behavior changes.