Feature/unwind specific consolidations while using update_or_create_hierarchy_from_dataframe() and Bug Fix #1174

[Mr-SabyasachiBose]

Feature addition

update_or_create_hierarchy_from_dataframe() currently unwinds the entire hierarchy when unwind param is set to True. A new param unwind_consol of type List is added in the function to unwind a list of specific consolidations in the hierarchy. if unwind is True and list of elements is provided to unwind_consol, it would unwind the valid consolidations from the list. If unwind_consol is empty and unwind is True, then executing the function would unwind entire hierarchy. if unwind is False, no unwind operation would execute.

Code update

Definition

def update_or_create_hierarchy_from_dataframe(
        self,
        dimension_name: str,
        hierarchy_name: str,
        df: 'pd.DataFrame',
        element_column: str = None,
        verify_unique_elements: bool = False,
        verify_edges: bool = True,
        element_type_column: str = 'ElementType',
        unwind: bool = False,
        unwind_consol: list = [],
        update_attribute_types: bool = False):

Unwind consolidation

    if unwind:
        if len(unwind_consol) == 0:
            self.remove_all_edges(dimension_name, hierarchy_name)
        else:
            for elem in unwind_consol:
                if self.elements.exists(dimension_name, hierarchy_name, elem):
                    self.remove_edges_under_consolidation(dimension_name, hierarchy_name, elem)    

Sample Execution

tm1_serv.hierarchies.update_or_create_hierarchy_from_dataframe(
        dimension_name="Sample Dim",
        hierarchy_name="Sample Dim",
        element_column="ElementID",
        df=df,
        unwind=True,
        unwind_consol=['Child2','parent   2',1,'PARENT1','Parent4']
)

Bug Fix

#1174

remove_edges_under_consolidation() wasn't unwinding an element if the name is in different in case and having different number of spaces (case and space sensitive).

Added/modified coding to allow the function to work in case and space insensitive nature.

    def remove_edges_under_consolidation(self, dimension_name: str, hierarchy_name: str,
                                         consolidation_element: str, **kwargs) -> List[Response]:
        """
        :param dimension_name: Name of the dimension
        :param hierarchy_name: Name of the hierarchy
        :param consolidation_element: Name of the Consolidated element
        :return: response
        """
        hierarchy = self.get(dimension_name, hierarchy_name)
        from TM1py.Services import ElementService
        element_service = ElementService(self._rest)
        elements_under_consolidations = element_service.get_members_under_consolidation(dimension_name, hierarchy_name,
                                                                                        consolidation_element)                                                            
        elements_under_consolidations.append(consolidation_element)
        elements_under_consolidations = [member.lower().replace(' ','') for member in elements_under_consolidations]
        remove_edges = []
        for (parent, component) in hierarchy.edges:    
            if parent.lower().replace(' ','') in elements_under_consolidations and component.lower().replace(' ','') in elements_under_consolidations:
                remove_edges.append((parent, component))
        hierarchy.remove_edges(remove_edges)
        return self.update(hierarchy, **kwargs)

[onefloid]

Hi @Mr-SabyasachiBose,

Thank you for sharing your code! I’ve left a review regarding the bug fix.

I also have a general tip for your commits. Your descriptions are good, but the commit titles could be more meaningful. Since most developers only glance at the title, it’s helpful to include a brief explanation of why you’re making these changes. For example, instead of:

Update HierarchyService.py

It would be clearer to use something like:

Fix: Compare case and space insensitively

You can see examples of commit messages from other developers in this project here.

[onefloid]

Have a look at Utils.py. It offers a lot of helper functions. So you can use lower_and_drop_spaces() for that. Using the helper function improves readability and reduces duplicated code.
Link

It would be better to have a separate commit for the bugfix, which only includes the changes for the bugfix. The benefits are:

• It’s easier to understand the commit history
• If there is case where the feature commits must be reverted, the fix remains.
• The maintainer of the code can cherry pick the bugfix to deliver the bugfix earlier or when your feature request would be declined.

Maybe it might also be worth to have a own branch/pull request to separate the bugfix from the new feature and allows independent merging.

[Mr-SabyasachiBose]

Thank you @onefloid, I will explore the function you suggested here for the purpose. Will try isolating the merge request as well.

[rkvinoth]

We shouldn't use a mutable default value, use None instead and handle the argument accordingly.

[rkvinoth]

According to PEP 8, if not unwind_consol is the right way to check for an empty list.

[MariusWirtz]

Thanks for the contributions @Mr-SabyasachiBose. Nice work!

Two comments on naming

• The arguments for TM1py functions are usually spelled out and not abbreviated. I think to be in line with the rest of the arguments, it should be unwind_consolidations

• I wonder if we should rename unwind to unwind_all. It would be very clear what it does and how it's different from unwind_consolidations. If we do this, we could ensure backward compatibility by using kwargs (check if unwind is passed in kwargs) and handling it accordingly.

[Mr-SabyasachiBose]

Thanks @MariusWirtz! I am totally buying unwind_all as that was something I thought of earlier but stepped back because of backward compatibility in mind.

we could add these to the code probably?

        # backward compatibility for unwind, the value for unwind would be assingned to unwind_all. expected type is bool
        if "unwind" in kwargs:
            unwind_all = kwargs["unwind"]

I have made a few updates as suggested by @onefloid and @rkvinoth about using existing utils functions and working with None or list data types. Also, adding additional two lines of code to enforce unwind_consolidation to be list only (if not None)