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

Add option to pass element selection as iterator in get_value function #814

Merged
merged 5 commits into from
Oct 14, 2022

Conversation

tobiaskapser
Copy link
Contributor

Hi together,

@MariusWirtz
We discussed the option to pass element coordinates as Iterators to the get_value()-function. That would remove the need to specify special separators for the element string.
You also had the idea to use the MDXpy Member object. I tried that and it worked out really nice! :)

It also simplifies the building of the MDX statement, because it provides the unique_name directly.
This could be used to create the Member directly from the splitted strings too. After that the MDX query can be build independently of the input type always from the Member object.
So I did some little restructuring to use the Member consistently throughout the function.

The get_value function is now callable without hierarchies like this:

get_value("CubeName", [(Dimension1, Element1), (Dimension2, Element2), (Dimension3, Element3)])

or with hierarchies like this:

get_value("CubeName", [(Dimension1, Hierarchy1, Element1), (Dimension1, Hierarchy2, Element2), (Dimension2, Element3)])

or directly with the mdxpy.Member objects

Unfortunately this has the disadvantage that the user has to always provide the dimension name. If not given it is not distinguishable if the next element belongs to the next dimension or to the same dimension but another hierarchy.
Does anyone see an option to determine that?

Elements can now be passed not only as string but also
as an Iterator. This prevents previous problems with choosing a
separator for the element string completely.
Use functionalities of the mdxpy.Member Object
to simplify construction of the MDX query.
@tobiaskapser tobiaskapser changed the title Add option to define element selection as iterator and some restructuring Add option to define element selection as iterator Oct 12, 2022
@tobiaskapser tobiaskapser changed the title Add option to define element selection as iterator Add option to pass element selection as iterator in get_value function Oct 12, 2022
@MariusWirtz
Copy link
Collaborator

Hi @tobiaskapser,

thanks! I like it.
It's nice that you added tests with the change.
I think the function is more useful now IMO.

It's kinda disturbing that the argument is called element_string when in reality it accepts a range of types besides strings. I guess we couldn't change this though without breaking backward compatability.

I will merge it as is.

@MariusWirtz
Copy link
Collaborator

Or maybe we could change it. Like we could introduce a new argument as position 2 and call it elements, members or intersection.

Then we could extract element_string from kwargs in case someone is passing the "old" keyword argument to the function.
That would be backward compatible I guess.

Not sure if it's worth it though only to have descriptive argument names...

@MariusWirtz
Copy link
Collaborator

Unfortunately this has the disadvantage that the user has to always provide the dimension name. If not given it is not distinguishable if the next element belongs to the next dimension or to the same dimension but another hierarchy. Does anyone see an option to determine that?

I could not think of a way to resolve this

@tobiaskapser
Copy link
Contributor Author

Or maybe we could change it. Like we could introduce a new argument as position 2 and call it elements, members or intersection.

Then we could extract element_string from kwargs in case someone is passing the "old" keyword argument to the function. That would be backward compatible I guess.

Not sure if it's worth it though only to have descriptive argument names...

That is a good idea! element_string really is misleading. I was also thinking about this problem while implementing it but did not came up with a solution.
I would implement this before we merge this. It is not a big change and would improve clarity.

@tobiaskapser
Copy link
Contributor Author

tobiaskapser commented Oct 14, 2022

Tested it. There is one problem with this approach:

Suppose the user passed element_string as a keyword argument, not as a positional argument. Like this:

get_value(cube_name, element_string="Element1,Element2,Element3")

This function call would be okay in the current version.

If we now change the second argument to elements (still positional) and extract the element_string from the kwargs the user gets an TypeError because the positional argument elements is not passed by the user.
That would break backwards compatibility for all function calls that pass the positional arguments as a keyword argument.

@MariusWirtz
Copy link
Collaborator

If we now change the second argument to elements (still positional) and extract the element_string from the kwargs the user gets an TypeError because the positional argument elements is not passed by the user.
That would break backwards compatibility for all function calls that pass the positional arguments as a keyword argument.

Would it work if we set the default value for elements to None then?

@tobiaskapser
Copy link
Contributor Author

That seems to work! I added the commits to my fork.

Provides more user friendly function interface.
The parameter "element_string" can still be accessed
through kwargs to provide backward compatibility.
@MariusWirtz MariusWirtz merged commit 4721254 into cubewise-code:master Oct 14, 2022
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.

2 participants