[Minor] Add kwargs to Plotting methods

[Xmaster6y]

What does this PR do?

It simply adds a kwargs argument to the Plotting methods.

Additionally

• performance_profile_figure should be renamed performance_profiles > impact docs & static assets

Let me know if this is of interest.

[matteobettini]

Thanbks! Definitely useful.
I don't think the name difference is too much of a problem but worst case we can create a performance_profiles function that just calls performance_profile_figure

[matteobettini]

I don't think we need to create a dict here, you can just add the kwargs

[Xmaster6y]

It allows for overriding the parameters like metric_name or save_tabular_as_latex on a per-call basis.

[matteobettini]

mmh then it would be better to have them as explicit args with defaults
def aggregate_scores(environment_comparison_matrix, metric_name=Plotting.METRIC_TO_PLOT, metrics_to_normalize=Plotting.METRICS_TO_NORMALIZE,save_tabular_as_latex=True, **kwargs):

[Xmaster6y]

Yes sure. Since they are class attributes the above syntax won't work but something like that should do

@staticmethod
def aggregate_scores(environment_comparison_matrix, metric_name=None, metrics_to_normalize=None, **kwargs):
    return aggregate_scores(
        dictionary=environment_comparison_matrix,
        metric_name=metric_name or Plotting.METRIC_TO_PLOT,
        metrics_to_normalize=metrics_to_normalize or Plotting.METRICS_TO_NORMALIZE,
        **kwargs,
    )

[matteobettini]

class Ciao:
    CIAO = "test"

    @staticmethod
    def test(stuff=CIAO):
        print(stuff)
        
Ciao.test()

test

it is working for me, am I missing smth?

[Xmaster6y]

Yes I was thinking about inheritance. But in fact what I did doesn't work either because we need classmethods.

The idea would be to do:

class CustomPlotting(Plotting):
    METRICS_TO_NORMALIZE = ["custom_metric"]
    METRIC_TO_PLOT = "custom_metric"

I updated the PR. Maybe the PR name should also be updated to reflect this. Let me know what you think.

[matteobettini]

Sorry I am not understanding. did you see the snippet in my previous comment? does it work for you?

[Xmaster6y]

Sorry for not being clear. Yes I saw it and answered based on it.

If you have:

class Ciao:
    CIAO = "test"

    @staticmethod
    def test(stuff=CIAO):
        print(stuff)

class CusomCiao(Ciao):
    CIAO = "custom"

Ciao.test()

then the result will be test instead of custom. But it works with the following:

class Ciao:
    CIAO = "test"

    @classmethod
    def test(cls):
        print(cls.CIAO)

class CusomCiao(Ciao):
    CIAO = "custom"

Ciao.test()

[matteobettini]

Check the PR now, and let me know if it runs. I think this is the simplest way to have defaults

[Xmaster6y]

Check the PR now, and let me know if it runs. I think this is the simplest way to have defaults

Well, not really... It does answer the kwargs issue (so I'm happy with it), but inheritance of defaults won't work this way, as I mentioned here: #195 (comment).

I am not sure to see the point of making a class if we can't use inheritance, but your call. Thanks anyway!

[matteobettini]

Ah now I understand, you were planning in case someone were to extend the class.

I don't think we need that kind of flexibility. This class is just a hook point to call the plotting library and only has static methods. if users want to change the class attributes (which should not be needed in benchmarl) they can edit them directly without any need to subclass.

[Xmaster6y]

Gotcha, thanks!