Update fstring

[KelvinChung2000]

Updating all the string.format with fstring to improve readability.

Minor formatting, such as removing some of the extra spaces.

[ahmedfgad]

Thanks @KelvinChung2000. It will save much space as the format() method is used over 600 times.

[KelvinChung2000]

It is more about readability and maintainability. I have another similar changes would like to make. Removing all the logging.error and use a single try except to capture the error. This in theory will make the code much clearer. Do you want me to do this on the same PR?

[ahmedfgad]

It is more about readability and maintainability. I have another similar changes would like to make. Removing all the logging.error and use a single try except to capture the error. This in theory will make the code much clearer. Do you want me to do this on the same PR?

Thanks @KelvinChung2000 for your suggestion.

But we cannot customize the error message using a single try-except block. I prefer to keep use logger.error() to customize the error message. This gives a clearer idea for the user about the issue to be fixed.

[KelvinChung2000]

You will still be able to have your customised message. Whenever you log the error, you raise the same message anyway. You can capture the raised error message at the except block and use a single logger.exception() for all the raised exceptions. Then you can do an exit(-1) to terminate the programme. Doing so, you can also remove the valid_parameters variable since the programme will be terminated at that __init__ point.

Which is something like this:

import logging

try:
    raise ValueError("This is a value error")
except Exception as e:
    logging.exception("Exception occurred")
    exit(-1)
    
# Output:
# ERROR: root: Exception occurred
# Traceback(most recent call last):
#   File "/GeneticAlgorithmPython/test.py", line 4, in <module >
#   raise ValueError("This is a value error")
# ValueError: This is a value error

Just a side note, the current valid_parameters is not providing a lot of utility since it will not be able to validate any parameter change after instantiation. I suggest moving all the check logic into a setter for each variable, so the checking can happen whenever the user is reassigning the variable. Having the separation, which should also improve the readability of the code. We can also add typing to the property, which allows for more tipping from IDE like vscode

Which can be something like this:

@property
def suppress_warnings(self) -> bool:
    return self._suppress_warnings

@suppress_warnings.setter
def suppress_warnings(self, suppress_warnings):
    # If suppress_warnings is bool and its value is False, then print warning messages.
    if type(suppress_warnings) is bool:
        self._suppress_warnings = suppress_warnings
    else:
        raise TypeError(
            f"The expected type of the 'suppress_warnings' parameter is bool but {type(suppress_warnings)} found.")

[ahmedfgad]

So, you suggest using try-except to have only one logger.error() statement for all checks?

Using setter would be a great addition too but we might consider it later.

[KelvinChung2000]

Yes, by doing so, you will not have any duplicated messages, which should make the code cleaner to read.

Changing to a setter should make the library more robust, but the migration will be a pain.

[ahmedfgad]

Yes, by doing so, you will not have any duplicated messages, which should make the code cleaner to read.

Changing to a setter should make the library more robust, but the migration will be a pain.

I totally agree. Using setter would solve some issues. But I will also introduce some conflictions for the user. This is a scenario.

Inside the class, we expect the value of a property named, for example, num_genes to be inside the protected variable _num_genes.

Sometimes the user modifies the properties outside the class. So, the user might change the value of num_genes outside the class. But it will not be used inside the class as we are using _num_genes instead.

For now, let's keep the setter aside.

[KelvinChung2000]

The changes will apply though to the protected variable, any assignments using self.num_genes = something will be assigning the value using the setter, which changes the _num_genes value. When the user reads the variable, the value will be returned from _num_genes , so in the user's perspective, this would not have changed anything but added robustness, as any potential false assignments will be caught.

[ahmedfgad]

The changes will apply though to the protected variable, any assignments using self.num_genes = something will be assigning the value using the setter, which changes the _num_genes value. When the user reads the variable, the value will be returned from _num_genes , so in the user's perspective, this would not have changed anything but added robustness, as any potential false assignments will be caught.

This makes sense.

But I am worried because our only motivation to use the setter and getter is to validate the parameters. But parameter validation happens only once. If the user have to call the getter method each time a property is retrieved, this would impact the performance. We are already suffering from high-computational time for some cases.

Also I think getter would be perfect in case that a logic must be applied when getting a property. In our case, we are just retrieving the property with no logic applied.

Maybe setter and getter would be applied to selected properties. An example is the gene_space. Sometimes it is given a value like gene_space=range(5). We might apply a code to unpack it whenever retrieved. So, instead of returning range(5), we might return the unpacked range [0, 1, 2, 3, 4].

[KelvinChung2000]

I am unsure how this will affect the performance; I will need to look this up. But I would guess this will induce some overhead. Let's leave it for now since this will be a substantial change which should not be included in this PR. Would you like the changes on the logger? If so, I will work on it later.

[ahmedfgad]

Right now, we do the following when an exception happens:

1. Raise an exception with the error message.
2. Log the error message using the logging module.

if ...:
    msg = "..."
    raise ValueError(msg)
    log.error(msg)
if ...:
    msg = "..."
    raise ValueError(msg)
    log.error(msg)

Both steps use an informational message that helps the user debug the issue.

If we are to separate the 2 messages so that we only use the log.error() only once, then the log message will not have the custom error message.

try:
    if ...:
        msg = "..."
        raise ValueError(msg)
    if ...:
        msg = "..."
        raise ValueError(msg)
except:
    log.error("Generic error message")

Some users would log all the messages to a file. I prefer to keep the error message in the log file so that they have a clear idea of what happened.

[KelvinChung2000]

You will still able to retain the error message. You can try out this example and you can see that message is being captured

import logging

try:
    raise ValueError("This is a custom value error")
except Exception as e:
    logging.exception(e)
    
# output
# ERROR:root:This is a custom value error
# Traceback(most recent call last):
#   File "/GeneticAlgorithmPython/test.py", line 4, in <module >
#   raise ValueError("This is a custom value error")
# ValueError: This is a custom value error

[ahmedfgad]

Great. Then please add it to the pull request.