search

PSLmodels / Business-Taxation

USA Corporate and Pass-Through Business Tax Model
11 stars 10 forks source link

About the Data.update_rescaling method #66

Closed martinholmer closed 5 years ago

martinholmer commented 5 years ago

When writing a test for the Data class update_rescaling method, I noticed a few improvement opportunities. Here is the code now:

    def update_rescaling(self, corplist, ncorplist):
        """
        Updates the rescaling factors associated with the DataFrame
        """
        assert len(corplist) == 14
        assert len(ncorplist) == 14
        self.rescale_corp = corplist
        self.rescale_noncorp = ncorplist

As far as I can see the two arguments passed in and both the Data class attributes rescale_?corp are all numpy arrays. Given that, wouldn't it be less confusing to replace corplist with something like corparray and replace ncorplist with something like ncorparray?

Also, I don't understand the docstring. What is meant by the phrase "factors associated with the DataFrame"? The factors are associated with the Data class, right? So, maybe it would be clearer to say something like this: Updates the rescaling factors in the Data class object

martinholmer commented 5 years ago

Issue #66 has been resolved by pull request #67.